Fixed Reader.HasNext() in Go client #3764
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jai1
reviewed
Mar 6, 2019
| @@ -1039,8 +1040,7 @@ void ConsumerImpl::hasMessageAvailableAsync(HasMessageAvailableCallback callback | |||
| return; | |||
| } | |||
|
|
|||
| BrokerGetLastMessageIdCallback callback1 = [this, lastDequed, callback](Result result, | |||
| MessageId messageId) { | |||
| getLastMessageIdAsync([this, lastDequed, callback](Result result, MessageId messageId) { | |||
| if (result == ResultOk) { | |||
There was a problem hiding this comment.
nit: can you refactor it to
const auto ret = (result == ResultOk && messageId > lastDequed && messageId.entryId() != -1);
callback(result, ret);
or even not use ret.
There was a problem hiding this comment.
I don't think that this would improve readability
jai1
reviewed
Mar 6, 2019
|
|
||
| client, err := NewClient(ClientOptions{ | ||
| URL: "pulsar://localhost:6650", | ||
| //Logger: func(level logutil.LoggerLevel, file string, line int, message string) { |
There was a problem hiding this comment.
Do you want to checkin with these comments
There was a problem hiding this comment.
Good point.. :) removing these
jai1
approved these changes
Mar 6, 2019
|
LGTM |
|
run java8 tests |
merlimat
added a commit
that referenced
this pull request
Mar 29, 2019
* Fixed Reader.HasNext() in Go client * Fixed formatting * Removed commented code
|
Merged in 2.3.1 at |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
In Go client, the
Reader.HasNext()is currently broken because it returns "true" when there are no more messages to read.The issue is in the C++
Consumer::hasMessageAvailable(), combined with the use of message listener. ThelastDequedMessageis being updated only after the listener is triggered, therefore an app checkinghasMessageAvailable()from the listener thread will always be 1 message behind.Since Go client wrapper, uses the listener internally, it is affected by this problem and it looks like a race condition with the
lastDequedMessagebeing updated in a background thread.