Get currentEnsemble from ledger rather than directly from metadata #1586

ivankelly · 2018-08-07T15:35:31Z

This will allow us to override the currentEnsemble being used during
recovery without modifying the metadata itself.

Master issue: #281

This will allow us to override the currentEnsemble being used during recovery without modifying the metadata itself. Master issue: apache#281

eolivelli

Looks good.
I think thr suggestions I made are around code that you are going to change soon, so this change looks good to me

+1

eolivelli · 2018-08-07T17:03:37Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java

@@ -1215,7 +1215,7 @@ private boolean isWritesetWritable(DistributionSchedule.WriteSet writeSet,

        int nonWritableCount = 0;
        for (int i = 0; i < sz; i++) {
-            if (!bk.getBookieClient().isWritable(getLedgerMetadata().currentEnsemble.get(i), key)) {
+            if (!bk.getBookieClient().isWritable(getCurrentEnsemble().get(i), key)) {


Sample currentEnsemble before starting the loop?

I am also in favor of saving the ensemble to a variable before starting the loop. However I think the logic here needs to be revisited, just to make sure ensemble changes won't impact the logic here.

changed. it won't affect the logic. this code is an advisary thing to apply backpressure. The ensemble could still change between a writeset being seen as writable, and the pendingAddOp being triggered, but this won't cause a problem.

eolivelli · 2018-08-07T17:05:13Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java

@@ -138,7 +138,7 @@ long getEntryId() {
    void sendWriteRequest(int bookieIndex) {
        int flags = isRecoveryAdd ? FLAG_RECOVERY_ADD | FLAG_HIGH_PRIORITY : FLAG_NONE;

-        lh.bk.getBookieClient().addEntry(lh.getLedgerMetadata().currentEnsemble.get(bookieIndex),
+        lh.bk.getBookieClient().addEntry(lh.getCurrentEnsemble().get(bookieIndex),


Sample the list outside of this method ? in the loop I mean

eolivelli · 2018-08-07T17:05:37Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java

@@ -266,7 +266,7 @@ public void writeComplete(int rc, long ledgerId, long entryId, BookieSocketAddre
        int bookieIndex = (Integer) ctx;
        --pendingWriteRequests;

-        if (!lh.getLedgerMetadata().currentEnsemble.get(bookieIndex).equals(addr)) {
+        if (!lh.getCurrentEnsemble().get(bookieIndex).equals(addr)) {


eolivelli · 2018-08-07T17:06:05Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadLacOp.java

-        for (int i = 0; i < metadata.currentEnsemble.size(); i++) {
-            lh.bk.getBookieClient().readLac(metadata.currentEnsemble.get(i),
+        for (int i = 0; i < currentEnsemble.size(); i++) {
+            lh.bk.getBookieClient().readLac(currentEnsemble.get(i),


This is perfect to me

@eolivelli can you elaborate why you made 'perfect' comment just for this? Just curious. :)

@jvrao I was exaggerating :-)

Only just because in the last past days for 2/3 times I had requested Ivan to sample currentEnsemble out of the loop, this time I did not add the comment.

sijie · 2018-08-07T17:29:49Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java

@@ -1215,7 +1215,7 @@ private boolean isWritesetWritable(DistributionSchedule.WriteSet writeSet,

        int nonWritableCount = 0;
        for (int i = 0; i < sz; i++) {
-            if (!bk.getBookieClient().isWritable(getLedgerMetadata().currentEnsemble.get(i), key)) {
+            if (!bk.getBookieClient().isWritable(getCurrentEnsemble().get(i), key)) {


I am also in favor of saving the ensemble to a variable before starting the loop. However I think the logic here needs to be revisited, just to make sure ensemble changes won't impact the logic here.

sijie · 2018-08-07T17:32:50Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java

+    List<BookieSocketAddress> getCurrentEnsemble() {
+        // Getting current ensemble from the metadata is only a temporary
+        // thing until metadata is immutable. At that point, current ensemble
+        // becomes a property of the LedgerHandle itself.


I know LedgerHandle is the mix of write and read handle. However this method is exposed to ReadOnlyHandle. It is a bit confused when we are saying current ensemble for a readonly handle. so I would suggest adding more comments here to clarify the meanings.

There'll be a big comment on the ReadOnlyLedgerHandle implementation. The whole class hierarchy here is broken. LedgerHandle should derive from ReadOnlyLedgerHandle, not vice versa. ReadOnlyLedgerHandle shouldn't do recovery. Recovery should be a separate operation that occurs before the ledger handle is even created. However, currently this is a big change because of the lack of separation between the *Op classes and LedgerHandle. This getCurrentEnsemble() is the ugliness I was referring to on dev@ when I said the solution isn't nice. The add op has to call back to the ledger handle to get the new ensemble after a failure. It would be better if, when an ensemble change occurs, all outstanding pending add ops are passed the new ensemble by the ledger handle. I might actually change it do that. The ops shouldn't depend on LedgerHandle. The objects they require should be passed in as parameters. There's so much janky stuff in this code, it can be painful to not fix it all at once.

@ivankelly we all understand the problem and motivation behind this. however that's a different topic to address all these. so let's not try to couple that here.

Here what I am asking is adding a comment to make thing clear. That's it. I want people who might be working on this file or when people look back at the git commit, we know exactly what was the thought of getCurrentEnsemble.

AddPendingOp now gets updates to the ensemble via "unsetSuccess..." Added a comment to getCurrentLedger about ROLedgerHandle's usage

eolivelli

Very clearer +1

eolivelli · 2018-08-08T12:01:45Z

Checkstyle is not happy

ivankelly · 2018-08-08T12:05:29Z

@eolivelli a lot of things are unhappy, and once I started looking into them, there's some more fundamental issues. Looking into them now.

Get currentEnsemble from ledger rather than directly from metadata

3e1f2fb

This will allow us to override the currentEnsemble being used during recovery without modifying the metadata itself. Master issue: apache#281

ivankelly self-assigned this Aug 7, 2018

ivankelly requested review from jvrao, merlimat and sijie August 7, 2018 15:35

eolivelli approved these changes Aug 7, 2018

View reviewed changes

sijie requested changes Aug 7, 2018

View reviewed changes

ivankelly added 2 commits August 7, 2018 20:53

review comments

670befa

Move calls to getCurrentLedger() into LedgerHandle

855389b

AddPendingOp now gets updates to the ensemble via "unsetSuccess..." Added a comment to getCurrentLedger about ROLedgerHandle's usage

eolivelli approved these changes Aug 8, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into current-ensemble

a951b78

sijie approved these changes Aug 21, 2018

View reviewed changes

ivankelly added this to the 4.9.0 milestone Aug 22, 2018

ivankelly added area/client release/4.9.0 type/improvement labels Aug 22, 2018

ivankelly closed this in 77b7fb8 Aug 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get currentEnsemble from ledger rather than directly from metadata #1586

Get currentEnsemble from ledger rather than directly from metadata #1586

ivankelly commented Aug 7, 2018

eolivelli left a comment

eolivelli Aug 7, 2018

sijie Aug 7, 2018

ivankelly Aug 7, 2018

eolivelli Aug 7, 2018

ivankelly Aug 7, 2018

eolivelli Aug 7, 2018

eolivelli Aug 7, 2018

jvrao Aug 9, 2018

eolivelli Aug 9, 2018

sijie Aug 7, 2018

sijie Aug 7, 2018

ivankelly Aug 7, 2018

sijie Aug 7, 2018

eolivelli left a comment

eolivelli commented Aug 8, 2018

ivankelly commented Aug 8, 2018

Get currentEnsemble from ledger rather than directly from metadata #1586

Get currentEnsemble from ledger rather than directly from metadata #1586

Conversation

ivankelly commented Aug 7, 2018

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli commented Aug 8, 2018

ivankelly commented Aug 8, 2018