Make LedgerHandle injectable #1589

ivankelly · 2018-08-08T23:48:28Z

This patch removes the hard dependency of LedgerHandle on BookKeeper.
This allows us to create an instance of LedgerHandle independently,
with a fully mocked BookieClient or LedgerManager and be clear on
where the objects used by the LedgerHandle are coming from.

This patch removes the hard dependency of LedgerHandle on BookKeeper. This allows us to create an instance of LedgerHandle independently, with a fully mocked BookieClient or LedgerManager and be clear on where the objects used by the LedgerHandle are coming from.

jvrao · 2018-08-08T23:52:35Z

What is the use-case of this change?

ivankelly · 2018-08-09T07:07:10Z

@jvrao testability of the client. In particular, I need fine grain control of when "bookies" and the "ledger-manager" respond to demonstrate a data loss bug (will push later today).

Currently, to get a ledger handle, you have to create a BookKeeper object, which creates zookeeper connections, bookie connections and a whole bunch of crap. These can be mocked with mockito, but this mocking is brittle; It's unclear as you are writing which objects you need to mock on the bookkeeper object. There's no defined interface between LedgerHandle and BookKeeper, so it ended up reaching into it's internals all over the place.

With this change, the interface between LedgerHandle and BookKeeper is explicit. If it changes in a breaking way, it will break at compile time, not when you run the tests.

I going to make mock BookieClient, BookieWatcher, and LedgerManager implementations, which can be used directly with LedgerHandle.

sijie

frankly speaking, I can't see the connection between this change and making ledger metadata immutable. The problem I see is we are accessing variables directly which make testing/mocking a bit hard. However if we just avoid direct access to variables and use getters, it would be more friendly to testing, and that's probably good enough for ledger metadata immutable change.

anyway I might be misreading this PR. if there is a strong reason about this PR for immutable metadata, please clarify it.

sijie · 2018-08-09T07:01:22Z

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

    }

    /**
     * Allow to extend BookKeeper for mocking in unit tests.
     */
    @VisibleForTesting
    BookKeeper() {
+        conf = new ClientConfiguration();
+        internalConf = ClientInternalConf.defaultValues();


since we construct a default conf already, better to use it rather than call defaultValues.

internalConf = ClientInternalConf.fromConfig(conf)

sijie · 2018-08-09T07:03:09Z

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

        metadataDriver = null;
-        readSpeculativeRequestPolicy = Optional.absent();


all the assignments in this constructor are for testing purpose. when you remove them, you basically changing the bookkeeper client behavior. I don't think it is a good idea do to so. in general, please don't do any behavior changes in a code refactor.

this is only used in one test, and I've rejigged that test to pass.

sijie · 2018-08-09T07:06:13Z

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

@@ -611,7 +544,7 @@ int getReturnRc(int rc) {
        }
    }

-    void scheduleBookieHealthCheckIfEnabled() {
+    void scheduleBookieHealthCheckIfEnabled(ClientConfiguration conf) {


since you are introducing an internal conf, why not make the behavior consistent?

I'd prefer to change this in a second pass where I make BookKeeper use ClientContext too. This way you can use a fully fledged bk client with all the outcalls mocked. It would also get rid of the need for a separate MockBookKeeper implementation.

sijie · 2018-08-09T07:14:52Z

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

@@ -101,9 +106,18 @@

    static final long PENDINGREQ_NOTWRITABLE_MASK = 0x01L << 62;

+    final ClientInternalConf conf;


I understand the motivation behind this PR. However I think this changes the memory footprint for a client that opens many ledgers. Because before this change, LedgerHandle points back to BK, where it is the shared place to access shared resources. Now we have increased so many fields for each ledger. I am kind of hesitated on this change.

I think what you need to change is just when LedgerHandle try to access shared resources in bk, it calls a method in bk, not access the field in bk directly. In this way, it always provides a clean path for mocking.

so I am not convinced (an more worried) on this change and I don't think it is really related to making ledger metadata immutable.

I think what you need to change is just when LedgerHandle try to access shared resources in bk, it calls a method in bk, not access the field in bk directly. In this way, it always provides a clean path for mocking.

What i want to do is get rid of the dependency on the concrete BookKeeper object. How we do it isn't so important. If you don't want these extra fields, we can create a ClientContext interface, which can be used to pull the stuff from bookkeeper. I think moving all the individual config flags to a single object still makes sense though.

so I am not convinced (an more worried) on this change and I don't think it is really related to making ledger metadata immutable.

It's only indirectly related to immutable metadata, but it does kinda block it. I think I found a data loss bug in deferred failure handling. I don't want to proceed with the metadata stuff until that bug is verified, fixed, and we have a solid test in place for it. Testing the bug depends on being able to control bookie, client and metadata interaction very tightly, which means injecting all of these. Hence this patch.

either ClientContext or ClientResources work.

Ok. Will rework it in morning

sijie · 2018-08-09T07:17:13Z

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

@@ -68,6 +71,8 @@
    boolean completed = false;

    LedgerHandle lh;
+    BookieClient bookieClient;


same concern as LedgerHandle. All these shared resources probably should come from one place, rather than introducing more fields here. Especially this is in critical write path.

sijie · 2018-08-09T07:18:20Z

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

@@ -68,6 +71,11 @@
    private final BitSet heardFromHostsBitSet;
    private final Set<BookieSocketAddress> sentToHosts = new HashSet<BookieSocketAddress>();
    LedgerHandle lh;
+    private final BookieClient bookieClient;


same concern as LedgerHandle. don't think it is a good idea to add 4 fields for each read op.

sijie · 2018-08-09T07:18:43Z

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

@@ -56,6 +59,14 @@

    final long requestTimeNano;
    private final LedgerHandle lh;
+    private final BookieClient bookieClient;


same concern as above

many variables

sijie

lgtm, just one comment that needs clarification.

sijie · 2018-08-14T17:50:19Z

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

+import org.apache.bookkeeper.meta.LedgerManager;
+import org.apache.bookkeeper.proto.BookieClient;
+
+interface ClientContext {


Can you add some comments?

sijie · 2018-08-14T17:59:17Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/client/LedgerRecoveryTest.java

-            }
-        }).parallelRead(true).readBatchSize(newConf.getRecoveryReadBatchSize());
+                    @Override
+                    public void operationComplete(int rc, Void result) {


I think this changes the testing behavior. the original testing behavior is parallelRead only enabled on LedgerRecoveryOp. but this PRs changes the behavior to apply to all behaviors on newBk, hences all recovery ops will inherit the settings. I am not sure is it okay or not, but I am not very conformatable about such behavior change in a refactor PR.

I suspect that it's not actually important for the test, but I've changed it to the exact original behaviour in any case.

sijie · 2018-08-15T07:29:17Z

@eolivelli can you review this?

@jvrao can anyone from Salesforce review this?

@jiazhai can you also spend some time on reviewing this? there is a bit code churn here, just wanna to have more eyes on it.

eolivelli · 2018-08-15T08:44:33Z

I am looking. It is a big change

eolivelli · 2018-08-15T08:46:53Z

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

@@ -1046,6 +1047,7 @@ private void replicateLedgerFragment(LedgerHandle lh,
        SingleFragmentCallback cb = new SingleFragmentCallback(
            resultCallBack,
            lh,
+            bkc.getMainWorkerPool(),


getClientCtx?

@eolivelli as I said in another comment, this patch I only want to focus on the LedgerHandle injectability. This line change is just because it was previously using the raw field. In a later pass, i'd like to get rid of all the methods like getMainWorkerPool from bookkeeper completely, but doing all that now would make an already huge patch even bigger.

Okay.
Patch looks good to me. Great work

eolivelli

Left just a nit. Overall seems okay

ivankelly · 2018-08-16T10:20:02Z

@jiazhai @jvrao could you folks take a look soon. this is blocking a whole stack of (smaller) PRs.

jiazhai · 2018-08-17T07:25:25Z

overall lgtm.
This PR seems mainly on using ClinentContext to unbind LedgerHandle and BookKeeper.
There seems will be a series changes following this PR. It would be better to open an issue or doc to describe what is the story behind this, and tracking all the related work, it will help the community know the big picture.

ivankelly · 2018-08-17T07:31:48Z

@jiazhai overall picture is to make metadata immutable on the client side (#281), which needs to be done to allow metadata upgrade to be done cleanly, and has been an outstanding bit of tech debt since 2014. This particular change was to allow a test to be written that triggered a data loss bug that I discovered during the immutable metadata work. The rest of the stack of patches is immutable metadata work.

ivankelly · 2018-08-20T19:19:13Z

@sijie @jvrao could I get some movement on this soon?

sijie · 2018-08-20T19:46:55Z

okay from me

jvrao · 2018-08-22T04:30:44Z

@dlg99 Can you please review?

ivankelly self-assigned this Aug 8, 2018

ivankelly requested review from jvrao, merlimat, sijie and eolivelli August 8, 2018 23:48

fix dlog build error

c80eb80

sijie requested changes Aug 9, 2018

View reviewed changes

ivankelly added 6 commits August 10, 2018 09:19

Wrap the stuff ledgers need into ClientContext to avoid storing too

ac79cbe

many variables

checkstyle

084d0be

sijie comment

339aebf

fix test

5aaa407

Merge remote-tracking branch 'origin/master' into injectable-lh

85c533a

Merge remote-tracking branch 'origin/master' into injectable-lh

bdf6f6f

sijie reviewed Aug 14, 2018

View reviewed changes

Sijie review comments

7fa5878

sijie requested a review from jiazhai August 15, 2018 07:29

sijie added type/task area/client labels Aug 15, 2018

eolivelli reviewed Aug 15, 2018

View reviewed changes

eolivelli approved these changes Aug 15, 2018

View reviewed changes

jiazhai approved these changes Aug 17, 2018

View reviewed changes

ivankelly mentioned this pull request Aug 20, 2018

(@bug W-5100764@) Write Ledger Handle listens for metadata changes #1580

Open

sijie approved these changes Aug 20, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into injectable-lh

88d5511

ivankelly added this to the 4.9.0 milestone Aug 21, 2018

ivankelly added release/4.9.0 type/improvement labels Aug 21, 2018

ivankelly closed this in 8e10328 Aug 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make LedgerHandle injectable #1589

Make LedgerHandle injectable #1589

ivankelly commented Aug 8, 2018

jvrao commented Aug 8, 2018

ivankelly commented Aug 9, 2018

sijie left a comment

sijie Aug 9, 2018

ivankelly Aug 10, 2018

sijie Aug 9, 2018

ivankelly Aug 10, 2018

sijie Aug 9, 2018

ivankelly Aug 10, 2018

sijie Aug 9, 2018

ivankelly Aug 9, 2018

sijie Aug 9, 2018

ivankelly Aug 9, 2018 •

edited

sijie Aug 9, 2018

sijie Aug 9, 2018

sijie Aug 9, 2018

sijie left a comment

sijie Aug 14, 2018

ivankelly Aug 14, 2018

sijie Aug 14, 2018

ivankelly Aug 14, 2018

sijie commented Aug 15, 2018

eolivelli commented Aug 15, 2018

eolivelli Aug 15, 2018

ivankelly Aug 15, 2018

eolivelli Aug 15, 2018

eolivelli left a comment

ivankelly commented Aug 16, 2018

jiazhai commented Aug 17, 2018

ivankelly commented Aug 17, 2018

ivankelly commented Aug 20, 2018

sijie commented Aug 20, 2018

jvrao commented Aug 22, 2018

		metadataDriver = null;
		readSpeculativeRequestPolicy = Optional.absent();

		@@ -101,9 +106,18 @@

		static final long PENDINGREQ_NOTWRITABLE_MASK = 0x01L << 62;

		final ClientInternalConf conf;

Navigation Menu

Make LedgerHandle injectable #1589

Make LedgerHandle injectable #1589

Conversation

ivankelly commented Aug 8, 2018

jvrao commented Aug 8, 2018

ivankelly commented Aug 9, 2018

sijie 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

ivankelly Aug 9, 2018 • edited

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

sijie 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

sijie commented Aug 15, 2018

eolivelli commented Aug 15, 2018

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

ivankelly commented Aug 16, 2018

jiazhai commented Aug 17, 2018

ivankelly commented Aug 17, 2018

ivankelly commented Aug 20, 2018

sijie commented Aug 20, 2018

jvrao commented Aug 22, 2018

ivankelly Aug 9, 2018 •

edited