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

Read ExplicitLAC in asyncReadLastConfirmed - enable ExplicitLAC in new API #1901

Merged
merged 7 commits into from
Oct 29, 2020

Conversation

eolivelli
Copy link
Contributor

  • Readers will handle explicit LAC transparently
  • V2 Readers stay with old PiggyBackLAC

Descriptions of the changes in this PR:
In readLastAddConfirmed we are going to call readLac which is an RPC supported from BK 4.5 which returns both the Piggybacked LAC and the Explicit LAC.
This way readers will be able to advance their view of the LAC even in case of writers which are using ExplicitLAC.
This change enables users of the new API to leverage ExplicitLAC

Client which are using v2 protocol will fallback to Piggybacked LAC as readLAC is not available in v2 protocol.

Motivation

This way new API users will be able to use ExplicitLAC

Changes

Call LedgerHandle#asyncReadExplicitLastConfirmed inside LedgerHandle#asyncReadLastConfirmed in case of non v2 protocol.


In order to uphold a high standard for quality for code contributions, Apache BookKeeper runs various precommit
checks for pull requests. A pull request can only be merged when it passes precommit checks. However running all
the precommit checks can take a long time, some trivial changes don't need to run all the precommit checks. You
can check following list to skip the tests that don't need to run for your pull request. Leave them unchecked if
you are not sure, committers will help you:

  • [skip bookkeeper-server bookie tests]: skip testing org.apache.bookkeeper.bookie in bookkeeper-server module.
  • [skip bookkeeper-server client tests]: skip testing org.apache.bookkeeper.client in bookkeeper-server module.
  • [skip bookkeeper-server replication tests]: skip testing org.apache.bookkeeper.replication in bookkeeper-server module.
  • [skip bookkeeper-server tls tests]: skip testing org.apache.bookkeeper.tls in bookkeeper-server module.
  • [skip bookkeeper-server remaining tests]: skip testing all other tests in bookkeeper-server module.
  • [skip integration tests]: skip docker based integration tests. if you make java code changes, you shouldn't skip integration tests.
  • [skip build java8]: skip build on java8. ONLY skip this when ONLY changing files under documentation under site.
  • [skip build java9]: skip build on java9. ONLY skip this when ONLY changing files under documentation under site.


Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

If this PR is a BookKeeper Proposal (BP):

  • Make sure the PR title is formatted like:
    <BP-#>: Description of bookkeeper proposal
    e.g. BP-1: 64 bits ledger is support
  • Attach the master issue link in the description of this PR.
  • Attach the google doc link if the BP is written in Google Doc.

Otherwise:

  • Make sure the PR title is formatted like:
    <Issue #>: Description of pull request
    e.g. Issue 123: Description ...
  • Make sure tests pass via mvn clean apache-rat:check install spotbugs:check.
  • Replace <Issue #> in the title with the actual Issue number.

- Readers will handle explicit LAC transparently
- V2 Readers stay with old PiggyBackLAC
@eolivelli
Copy link
Contributor Author

run bookkeeper-server client tests

Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

My take with this - delay merging this to 4.10. since we are almost ready for 4.9 (waiting for the binary metadata change).

it is a behavior change, I would prefer us having a set of BC tests covered this case. otherwise I am not comfortable at merging this PR in last minute.

@eolivelli
Copy link
Contributor Author

@sijie do you think this narrower patch could be a good tradeoff ?
I am not changing any protocol, no new concept.

@eolivelli
Copy link
Contributor Author

@sijie sorry I did not see your comment

@eolivelli
Copy link
Contributor Author

Okay @sijie I will wait for 4.10.
I have other changes about ExplicitLAC, let's release 4.9 first.

@eolivelli
Copy link
Contributor Author

run bookkeeper-server client tests

@eolivelli
Copy link
Contributor Author

I will send a BP with a more complete view.

@eolivelli eolivelli closed this Feb 21, 2019
@sijie
Copy link
Member

sijie commented Apr 24, 2019

@eolivelli : @zymap was working on fixing a problem for apache/pulsar#3828. so we might need this change. I am wondering if you are still working on this. If you already have a proposal but not start implementing it, @zymap might be able to pick up the task from your side.

@eolivelli
Copy link
Contributor Author

@sijie. It is in my backlog but not a priority. So please pick it up. Thanks

@eolivelli
Copy link
Contributor Author

@zymap @sijie do you want to pick this up ?
it would be a great step forward for the new API, but I don't have much time

@jiazhai jiazhai reopened this Oct 20, 2020
@eolivelli
Copy link
Contributor Author

@jiazhai are you interested in this work ?
If you want I can rebase to current master

cc @sijie

@sijie
Copy link
Member

sijie commented Oct 21, 2020

@eolivelli Yes. If you can rebase to current master, that would be super helpful! Much appreciated!

@jiazhai
Copy link
Member

jiazhai commented Oct 26, 2020

@zymap have verified this change worked well in the Pulsar env. And a test is added in pulsar repo, once pulsar has this change, we could enable that test.

@jiazhai
Copy link
Member

jiazhai commented Oct 26, 2020

@sijie would you please help review this change again, there was a "change requested", and it may block the pr merge

@eolivelli
Copy link
Contributor Author

@jiazhai I have merged with current master.
It looks like the change is so little that there was no conflict at all

if you and @sijie agree on the approach it is better that I finish the work and add more tests

@jiazhai
Copy link
Member

jiazhai commented Oct 27, 2020

Thanks @eolivelli , +1 for this approch

@eolivelli eolivelli added this to the 4.12.0 milestone Oct 27, 2020
@eolivelli eolivelli self-assigned this Oct 27, 2020
@eolivelli
Copy link
Contributor Author

@jiazhai @sijie @zymap in Pulsar we are using DBLedgerStorage

Probably in order to fully leverage this feature we have to fix this issue (I found a reference to it in testExplicitLACIsPersisted

#1533

Copy link
Contributor Author

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

@sijie @jiazhai the patch is ready for review.

Please @sijie check it out, you left "request changes" status here

@jiazhai jiazhai merged commit 66912fd into apache:master Oct 29, 2020
@sijie
Copy link
Member

sijie commented Oct 30, 2020

@eolivelli @jiazhai Can you cherry-pick this change to branch-4.11 and release 4.11.1? I'd like to land this change for fixing the presto read last message issue in Pulsar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants