You are viewing a plain text version of this content. The canonical link for it is here.
Posted to devnull@infra.apache.org by "sijie (GitHub)" <gi...@apache.org> on 2018/08/22 21:18:18 UTC
[GitHub] [bookkeeper] sijie opened pull request #1619: Provide async
version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Descriptions of the changes in this PR:
### Motivation
Auditor has multiple places calling sync methods in async callbacks.
This raises the possibility hitting deadlock. Issue #1578 is one of the examples.
After looking into the `LedgerUnderreplicationManager`, `markLedgerUnderreplicated`
is the only interface that will be called in async callbacks. This change is
to provide an async version of `markLedgerUnderreplicated`.
### Changes
- add `markLedgerUnderreplicatedAsync` interface in `LedgerUnderreplicationManager`.
- implement the logic of `markLedgerUnderreplicated` using async callbacks
- use `markLedgerUnderreplicatedAsync` in the Auditor
Related Issues: #1578
Master Issue: #1617
> ---
> 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.
>
> ---
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] reddycharan commented on pull request #1619:
Provide async version of markLedgerUnderreplicated for
LedgerUnderreplicationManager
Posted by "reddycharan (GitHub)" <gi...@apache.org>.
nit: is there any need of making 'missingReplicas' collection, instead of just String missingReplica
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie commented on issue #1619: Provide async
version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
> change of this magnitude needs some new test coverage for confidence.
if there is no functionality change, what new tests do you expect?
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] reddycharan commented on pull request #1619:
Provide async version of markLedgerUnderreplicated for
LedgerUnderreplicationManager
Posted by "reddycharan (GitHub)" <gi...@apache.org>.
nit: dont we need to have async version of "void markLedgerReplicated(long ledgerId)"
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie commented on pull request #1619: Provide
async version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
check here: https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java#L584
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie commented on issue #1619: Provide async
version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
regarding tests: this PR only focuses on rewriting `markLedgerUnderreplicated` in async way. it doesn't change existing logic or adding new functionalities, so no tests are added.
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie commented on issue #1619: Provide async
version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
@jvrao @reddycharan @merlimat
This PR is to follow up the comment in #1608. After looking into LedgerUnderreplicationManager, I think `markLedgerUnderreplicated` is the only method will be called in async callbacks. so the change is pretty straightforward to rewrite `markLedgerUnderreplicated` in async method.
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] reddycharan commented on issue #1619: Provide
async version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "reddycharan (GitHub)" <gi...@apache.org>.
changes LGTM, but i understand that there is no functional change and they are not public API methods, but change of this magnitude needs some new test coverage for confidence.
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie commented on pull request #1619: Provide
async version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
processFuture is completed only when finished processing the last ledger, so checkAllLedgers is still sync, which is fine, since checkAllLedgers is executing in Auditor's own executor.
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] reddycharan commented on pull request #1619:
Provide async version of markLedgerUnderreplicated for
LedgerUnderreplicationManager
Posted by "reddycharan (GitHub)" <gi...@apache.org>.
FutureUtils.result would throw InterruptedException without applying the exceptionHandler function https://github.com/apache/bookkeeper/blob/master/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/concurrent/FutureUtils.java#L77
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie commented on pull request #1619: Provide
async version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
we don't. `markLedgerReplicated` is used by replication worker, which replication worker is single threaded and using sync methods. so `markLedgerReplicated` is fine at the context.
only `markLedgerUnderreplicated` is the issue.
ideally, LedgerUnderreplicationManager should be splitted into at least 2 interfaces, one for Auditor, the other one for ReplicationWorker. That would make things much clearer.
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] reddycharan commented on pull request #1619:
Provide async version of markLedgerUnderreplicated for
LedgerUnderreplicationManager
Posted by "reddycharan (GitHub)" <gi...@apache.org>.
FutureUtils.result would throw InterruptedException without applying the exceptionHandler function https://github.com/apache/bookkeeper/blob/master/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/concurrent/FutureUtils.java#L77
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] reddycharan commented on pull request #1619:
Provide async version of markLedgerUnderreplicated for
LedgerUnderreplicationManager
Posted by "reddycharan (GitHub)" <gi...@apache.org>.
why is it responsibility of ProcessLostFragmentsCb to close the ledgerhandle, I'm not sure if it is appropriate to do it here, since ProcessLostFragmentsCb is not the owner of 'lh'
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] reddycharan commented on pull request #1619:
Provide async version of markLedgerUnderreplicated for
LedgerUnderreplicationManager
Posted by "reddycharan (GitHub)" <gi...@apache.org>.
checkLedgersProcessor
closes the 'lh' https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java#L679
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie commented on issue #1619: Provide async
version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
This test is only rewriting sync calls to use async calls. Although `markLedgerUnderreplicatedAsync` is an async method, it is still used in sync ways across the places at Auditor. That says this PR doesn't change any existing logic how Auditor uses `markLedgerUnderreplicated`. from functionality wises, we don't need separate test cases.
Agreed that we need to have better coverage and since `makrLedgerUnderreplicatedAsync` is now at the interface, people might be using it in a different place. Those should be done in a separate PR rather than this PR. so I created a separate issue for tracking test coverage: https://github.com/apache/bookkeeper/issues/1626, not trying to combine changes with different purposes into one PR.
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie commented on pull request #1619: Provide
async version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
nice. I would remove this here
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie commented on pull request #1619: Provide
async version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
I didn’t change the behavior. The original logic is ProcessLostFramgmentsCb closing it.
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] reddycharan commented on pull request #1619:
Provide async version of markLedgerUnderreplicated for
LedgerUnderreplicationManager
Posted by "reddycharan (GitHub)" <gi...@apache.org>.
is there any need of making 'missingReplicas' collection, instead of just String missingReplica
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie commented on pull request #1619: Provide
async version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java#L679
this line here is wrong.
However it was "okay" because close in ReadOnlyHandle is no-op.
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] reddycharan commented on pull request #1619:
Provide async version of markLedgerUnderreplicated for
LedgerUnderreplicationManager
Posted by "reddycharan (GitHub)" <gi...@apache.org>.
just want to make sure that this 'cause' is not lost in log errors. will it not hide cause completely here?
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie commented on pull request #1619: Provide
async version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
yeah. I added the logging back. please take a look at the latest commit.
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] merlimat commented on pull request #1619:
Provide async version of markLedgerUnderreplicated for
LedgerUnderreplicationManager
Posted by "merlimat (GitHub)" <gi...@apache.org>.
If processing is done async, will get impacted by the below `admin.close()` and `client.close()`?
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie commented on issue #1619: Provide async
version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
run bookkeeper-server replication tests
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie commented on pull request #1619: Provide
async version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
I changed this for https://github.com/apache/bookkeeper/pull/1619/files#diff-7525f06ad3a1ad0a00a462df4deb4698L581 . then it doesn't need to do multiple zk calls for creating an UL.
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie commented on pull request #1619: Provide
async version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
this allows the method `tryMarkLedgerUnderreplicatedAsync` to be attempted with same `CompleteableFuture`.
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] reddycharan commented on pull request #1619:
Provide async version of markLedgerUnderreplicated for
LedgerUnderreplicationManager
Posted by "reddycharan (GitHub)" <gi...@apache.org>.
nit: why does caller has to create 'createFuture' (CompletableFuture) and pass it, why cann't method returns the CompletableFuture
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie closed pull request #1619: Provide async
version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
[ pull request closed by sijie ]
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [bookkeeper] sijie commented on issue #1619: Provide async
version of markLedgerUnderreplicated for LedgerUnderreplicationManager
Posted by "sijie (GitHub)" <gi...@apache.org>.
@reddycharan I have addressed your comments.
[ Full content available at: https://github.com/apache/bookkeeper/pull/1619 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org