-
Notifications
You must be signed in to change notification settings - Fork 345
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
Support nack backoff policy for SDK #660
Changes from 10 commits
7efb2be
d471316
16c0912
4fa4e50
343a28b
c970183
9ca14c6
89dcd77
6bf2991
014fe8d
2fc332f
b77b314
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,22 +35,41 @@ type negativeAcksTracker struct { | |
doneOnce sync.Once | ||
negativeAcks map[messageID]time.Time | ||
rc redeliveryConsumer | ||
tick *time.Ticker | ||
nackBackoff NackBackoffPolicy | ||
trackFlag bool | ||
delay time.Duration | ||
log log.Logger | ||
} | ||
|
||
func newNegativeAcksTracker(rc redeliveryConsumer, delay time.Duration, logger log.Logger) *negativeAcksTracker { | ||
t := &negativeAcksTracker{ | ||
doneCh: make(chan interface{}), | ||
negativeAcks: make(map[messageID]time.Time), | ||
rc: rc, | ||
tick: time.NewTicker(delay / 3), | ||
delay: delay, | ||
log: logger, | ||
} | ||
func newNegativeAcksTracker(rc redeliveryConsumer, delay time.Duration, | ||
nackBackoffPolicy NackBackoffPolicy, logger log.Logger) *negativeAcksTracker { | ||
|
||
t := new(negativeAcksTracker) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this can just be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are the same effect |
||
|
||
// When using NackBackoffPolicy, the delay time needs to be calculated based on the RedeliveryCount field in | ||
// the CommandMessage, so for the original default Nack() logic, we still keep the negativeAcksTracker created | ||
// when we open a gorutine to execute the logic of `t.track()`. But for the NackBackoffPolicy method, we need | ||
// to execute the logic of `t.track()` when AddMessage(). | ||
if nackBackoffPolicy != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a little confused on why we need an if statement. Shouldn't the default Implementation of the
Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, agree with your point of view. The problem here is because, for nackbackoff, we can't directly get the corresponding nackDelayTime, we need to get the redeliveryCount through the CommandMessage and then calculate the nackDelayTime, then we can determine the time.NewTicker based on the nackDelayTime. It is precisely because of such a relationship that the if statement is added |
||
t = &negativeAcksTracker{ | ||
doneCh: make(chan interface{}), | ||
negativeAcks: make(map[messageID]time.Time), | ||
nackBackoff: nackBackoffPolicy, | ||
rc: rc, | ||
log: logger, | ||
} | ||
} else { | ||
t = &negativeAcksTracker{ | ||
doneCh: make(chan interface{}), | ||
negativeAcks: make(map[messageID]time.Time), | ||
rc: rc, | ||
nackBackoff: nil, | ||
delay: delay, | ||
log: logger, | ||
} | ||
|
||
go t.track() | ||
go t.track(time.NewTicker(t.delay / 3)) | ||
} | ||
return t | ||
} | ||
|
||
|
@@ -76,14 +95,48 @@ func (t *negativeAcksTracker) Add(msgID messageID) { | |
t.negativeAcks[batchMsgID] = targetTime | ||
} | ||
|
||
func (t *negativeAcksTracker) track() { | ||
func (t *negativeAcksTracker) AddMessage(msg Message) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is there a new method here? Also, it looks like state is changing here without a lock. If multiple go routines call this at once multiple tracking routines could be started right? Can the tracking go routine just be started at creation time? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we need to get redeliveryCount through the Message interface |
||
nackBackoffDelay := t.nackBackoff.Next(msg.RedeliveryCount()) | ||
t.delay = time.Duration(nackBackoffDelay) | ||
|
||
// Use trackFlag to avoid opening a new gorutine to execute `t.track()` every AddMessage. | ||
// In fact, we only need to execute it once. | ||
if !t.trackFlag { | ||
go t.track(time.NewTicker(t.delay / 3)) | ||
t.trackFlag = true | ||
} | ||
|
||
msgID := msg.ID() | ||
|
||
// Always clear up the batch index since we want to track the nack | ||
// for the entire batch | ||
batchMsgID := messageID{ | ||
ledgerID: msgID.LedgerID(), | ||
entryID: msgID.EntryID(), | ||
batchIdx: 0, | ||
} | ||
|
||
t.Lock() | ||
defer t.Unlock() | ||
|
||
_, present := t.negativeAcks[batchMsgID] | ||
if present { | ||
// The batch is already being tracked | ||
return | ||
} | ||
|
||
targetTime := time.Now().Add(time.Duration(nackBackoffDelay)) | ||
t.negativeAcks[batchMsgID] = targetTime | ||
} | ||
|
||
func (t *negativeAcksTracker) track(ticker *time.Ticker) { | ||
for { | ||
select { | ||
case <-t.doneCh: | ||
t.log.Debug("Closing nack tracker") | ||
return | ||
|
||
case <-t.tick.C: | ||
case <-ticker.C: | ||
{ | ||
now := time.Now() | ||
msgIds := make([]messageID, 0) | ||
|
@@ -105,15 +158,13 @@ func (t *negativeAcksTracker) track() { | |
t.rc.Redeliver(msgIds) | ||
} | ||
} | ||
|
||
} | ||
} | ||
} | ||
|
||
func (t *negativeAcksTracker) Close() { | ||
// allow Close() to be invoked multiple times by consumer_partition to avoid panic | ||
t.doneOnce.Do(func() { | ||
t.tick.Stop() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is the ticker getting cleanup now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the current implementation situation, if we use the t.ticker in the struct, there will be a data race, so now we use the temporary variables of the ticker, and there is no good way to see how to close the temporarily created ticker. |
||
t.doneCh <- nil | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed
EnableDefaultNackBackoffPolicy
?. If theNackBackoffPolicy
is not supplied we can just the default?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no
EnableDefaultNackBackoffPolicy
, it will invade the existing code logic. When the NackBackoffPolicy policy is empty, suppose we use the default NackBackoffPolicy, then when the user uses the Nack(Message) interface, the new implementation will be used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me a cleaner API is to just have
NackBackoffPolicy
and expose the basic/default policy. If the policy is not set than it uses the current behavior. This way there is only 1 configuration knob to worry about.