Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mark processor down for some time not updating resolve ts #159

Closed
wants to merge 8 commits into from

Conversation

july2993
Copy link
Contributor

@july2993 july2993 commented Dec 5, 2019

What problem does this PR solve?

review after #154

mark processor down for some time not updating resolve ts

What is changed and how it works?

mark processor down for some time not updating resolve ts.
and clear up data dispatch tables to other captures laster.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

@amyangfei
Copy link
Contributor

#154 is merged now, pls resolved the conflict

@july2993 july2993 added the status/ptal Could you please take a look? label Dec 5, 2019
}

func (o *ownerImpl) removeCapture(info *model.CaptureInfo) {
o.l.Lock()
delete(o.captures, info.ID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use defer to unlock here?

}

oldPinfo, ok := cfInfo.ProcessorInfos[cid]
if !ok || oldPinfo.ResolvedTs != pinfo.ResolvedTs {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both ResolvedTs and CheckpointTs change can trigger the update of last update time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only check ResolvedTS here do you means check it both?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if !ok || oldPinfo.ResolvedTs != pinfo.ResolvedTs || oldPinfo.CheckpointTs != pinfo.CheckpointTs
like this?

@@ -34,6 +35,8 @@ import (
"go.uber.org/zap"
)

var markProcessorDownTime = time.Minute
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the interval that processor updates resolvedTs or checkpointTs is not always less than 1 minute. Besides shall we allow several times update failure of processor and do we need to increase this value?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/ptal Could you please take a look?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants