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
Fixed simultaneus reads on same ledger/entry with v2 protocol #720
Conversation
FAILURE --none-- |
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.
+1 #shipit
* | ||
* @throws Exception | ||
*/ | ||
@Test(timeout = 20000) |
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.
Remove timemout?
I recall there was a require to remove all the "timeout", since we have a global timeout set in surefire configuration.
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.
Sure, will update
* | ||
* @throws Exception | ||
*/ | ||
@Test(timeout = 20000) |
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.
same here
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.
+1, LGTM, only minor comments.
@@ -845,6 +846,56 @@ private void writeAndFlush(final Channel channel, | |||
errorOut(key); | |||
} | |||
} | |||
|
|||
void errorOutReadKey(final CompletionKey key) { |
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.
This code isn't valid anymore. Erroring out is now part of the completionValue, so this should be handled there.
} | ||
} | ||
|
||
final Channel c = channel; |
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.
This is handled in writeAndFlush now. You'll need to add a check for the conflicts map into the #errorOut method.
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.
Ok, I've missed that part. I'll fix that
@merlimat @ivankelly : any updates on this PR? I was lost who is picking up pr. |
I made a PR into matteo's branch. When he merges that, this will be ready
for review again
…On Mon 4 Dec 2017, 08:43 Sijie Guo ***@***.***> wrote:
@merlimat <https://github.com/merlimat> @ivankelly
<https://github.com/ivankelly> : any updates on this PR? I was lost who
is picking up pr.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#720 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADWqxD7lmLtY82ddCv3dIlQkH87zRQvks5s86KngaJpZM4QaZxa>
.
|
73473f6
to
d8a14e3
Compare
Rebased and applied fixes from @ivankelly |
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.
LGTM
This change is coming from yahoo branch at YahooArchive/bookkeeper@ce7b0f7
With v2 protocol there's no way to disambiguate the read request since the completion key is made from (ledgerId, entryId).
This PR introduced an additional multimap to handle the conflicts (multiple simultaneus read requests for the same entry).