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

separate transfer queue to active and standby processor #661

Merged
merged 14 commits into from
Apr 24, 2018

Conversation

wxing1292
Copy link
Contributor

@wxing1292 wxing1292 commented Apr 11, 2018

separate queue ack mgr into separate file

Review #648 first, since this PR contains some code from #648

solve #564
partially solve #566

@coveralls
Copy link

coveralls commented Apr 11, 2018

Coverage Status

Coverage increased (+0.4%) to 65.685% when pulling f7e6b01 on transfer-Q-ack-x into b2f53a9 on master.

separate queue ack mgr into separate file
@wxing1292 wxing1292 force-pushed the transfer-Q-ack-x branch 3 times, most recently from 0571cf6 to e230c92 Compare April 12, 2018 19:02

// error which will be thrown if the timer / transfer task should be
// retries due to various of reasons
taskRetryError struct{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's reuse the same error used by timerQueueProcessor.

}

transferQueueProcessor interface {
common.Daemon
NotifyNewTask()
NotifyNewTask(clusterName string)
SetCurrentTime(clusterName string, currentTime time.Time)
Copy link
Contributor

Choose a reason for hiding this comment

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

TransferQueueProcessor does not use time at all. It uses a constant value for visibilityTime for all transfer and replicator tasks.
What's the purpose of this API?

Copy link
Contributor Author

@wxing1292 wxing1292 Apr 18, 2018

Choose a reason for hiding this comment

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

this is to delay the processing of the standby transfer tasks.

@@ -118,12 +132,11 @@ type (
}

timerQueueAckMgr interface {
readRetryTimerTasks() []*persistence.TimerTaskInfo
getFinishedChan() <-chan struct{}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you will remove it from this PR. I think the timer changes covers this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

plz ignore the timer* files

@@ -99,6 +109,8 @@ func (p *replicatorQueueProcessorImpl) Process(qTask queueTaskInfo) error {

if err != nil {
p.metricsClient.IncCounter(scope, metrics.TaskRequests)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this to TaskFailure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@@ -99,6 +109,8 @@ func (p *replicatorQueueProcessorImpl) Process(qTask queueTaskInfo) error {

if err != nil {
p.metricsClient.IncCounter(scope, metrics.TaskRequests)
} else {
p.queueAckMgr.completeTask(task.TaskID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why move this out of base processor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i believe there should be some logical separation of base processor and actual processor.
the base processor should only deal with retry, when seeing a error, and the actual task processor (the actual processing logic) should deal with errors such as entity not exists.

@wxing1292 wxing1292 merged commit c3e240e into master Apr 24, 2018
@wxing1292 wxing1292 deleted the transfer-Q-ack-x branch April 24, 2018 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants