ZOOKEEPER-3495: fix SnapshotDigestTest to work with JDK12+ #1056

symat · 2019-08-15T16:45:22Z

The problem with the test SnapshotDigestTest.testDifferentDigestVersion was that it used reflection to change a final static value in DigestCalculator, what is no longer supported with JDK 12+ (see JDK-8210522)

I think there are still some hacky solutions to go behind these limitations (with PowerMock maybe or using VarHandle as suggested here), but I think the best is to avoid the situation when we need to poke the static final field.

I changed the fully static DigestCalculator, converting it to a static singleton object with non-static methods / fields. The fields of the singleton object can not be modified from production code, but we can change them from the tests even in JDK 12 / 13 (as they are not static final fields).

I tested it locally using openjdk, I hope it will also work in jenkins.

eolivelli

The approach looks good to me.
I left a comment.

I have spent a few cycles on this problem during jdk12 early access stage.
There would be other tricks to tweak constant values in tests, like https://github.com/eolivelli/tweakjava

But in this case it is better to use your approach that will be valid for the long term

eolivelli · 2019-08-15T17:21:09Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/util/DigestCalculator.java

    }

+


This is not final, why don't you add a setDigestEnabled method?
Make it 'volatile'

We won't lose much in performances

anmolnar

This patch still uses reflection, so it seems to me that we're shooting our other leg. I can be convinced though, but I think there must be a very cool design pattern for this which we can test easily without using reflection. See my comments first and let me think about it.

zookeeper-server/src/test/java/org/apache/zookeeper/server/DigestCalculatorTestUtil.java

anmolnar · 2019-08-16T09:06:57Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/util/DigestCalculator.java


+    public static final DigestCalculator DIGEST_CALCULATOR;


What if we make DigestCalculator interface, remove final from here and mock it easily in tests?
What's the benefit of making it final?

I think that would work too, and mocking is more elegant than using plain reflection in the tests. (I think with Mockito you can even mock the class and you don't even need an intreface)

The only issue with the removal of the final modifier is that from that point anywhere from the production code you can easily change this singleton. But we can write a comment that it is to be changed only from tests...

eolivelli · 2019-08-16T09:32:15Z

@anmolnar I suggested to add a setter method, so mo more need for reflection

symat · 2019-08-16T10:10:49Z

@eolivelli

I suggested to add a setter method, so mo more need for reflection

if we want to avoid both reflection and mocking, then we actually need two setter methods, one for the setEnable and one for the setVersion. Both of the fields can be made volatile in this case I think.

we can add @VisibleForTesting on the setter methods - do we use that in Zookeeper? I found it, but always it is commented out.

eolivelli · 2019-08-16T11:13:57Z

For VisibleForTesting follow the examples you found, we don't have a rule yet

anmolnar · 2019-08-16T13:25:03Z

I think this code is not perfectly organised:

DigestCalculator is a dependency of DataTree, so it should be moved to the same package and all members can be package-private which makes them unaccessible from outside,
We can introduce a public interface making interface members public and letting unit tests mocking without reflection.
DataTree constructor can be overloaded with one which will get DigestCalculator interface as a dependency, so the entire static approach can be removed. Tests can use the new constructor to inject the mocked version of the Calculator this way.

symat · 2019-08-16T15:12:53Z

I think this code is not perfectly organised:
...

good idea, I will try this way

lvfangmin · 2019-08-20T18:53:23Z

The DIGEST_VERSION is made to be static final to make sure it won't be changed in code with things like setVersion.

It seems to be safer and cleaner to me with the current code implementation, for the test if we want to avoid reflection we add a pre-defined snapshot file with different digest version (like -1), and test the difference version case.

Also DigestCalculator is not only used in DataTree, it will also being used in PrepRequestProcessor when adding real time digest on txn, so this class is more about a util which could be replaced with different digest method. I'm preparing the PR in the last few days, will send the PR probably today.

symat · 2019-08-21T12:34:55Z

Also DigestCalculator is not only used in DataTree, it will also being used in PrepRequestProcessor ...

Thanks for pointing this out! Let's wait for your PR to be submitted / merged, before finalizing this one. Can you link the jira ticket / PR here?

The DIGEST_VERSION is made to be static final to make sure it won't be changed in code with things like setVersion.

I think with the solution proposed by @anmolnar it is still possible to keep the DIGEST_VERSION to be static final and make it impossible to change it from the code accidentally. We only create a default constructor for DataTree, injecting a DigestCalculator which is using the static final DIGEST_VERSION. While for some tests, we can create a DataTree with a mocked DigestCalculator. (I think this approach is cleaner in terms of object-oriented design patterns, as the DigestCalculator is actually a dependency of the DataTree, but it is hidden in the current implementation.)

I will submit a new commit here, trying out @anmolnar's proposal, then we can see if everyone likes it or not :) - but let's wait with the merge until I merged with @lvfangmin's change...

anmolnar · 2019-08-21T19:59:57Z

@lvfangmin

The DIGEST_VERSION is made to be static final to make sure it won't be changed in code with things like setVersion.

Would you please be a little bit more specific here? I don't see what code changes are u trying to protect DIGEST_VERSION from and why would my proposal brake that? I see the opposite: making it package-private is as safe as static final while it makes the code cleaner and testable at the same time.

It seems to be safer and cleaner to me with the current code implementation...

Why? Could you please be more specific?
In my opinion using interfaces with instantiated objects are cleaner and more unit test friendly.

Also DigestCalculator is not only used in DataTree, it will also being used in PrepRequestProcessor

No problem with that. It can still use the interface to be injected as constructor dependency same way as DataTree should use it.

I'm looking forward to your patch and see how unit testing is carried out. I really want to avoid reflection everywhere, because we can clearly see now how fragile it could be. Additionally your proposal about preparing suitable snapshot files could be working, but that also breaks unit testing principles.

Let's see @symat 's updated patch and talk about which way to go forward.

anmolnar

Getting better. I still think using interface would make the code even more cleaner.

zookeeper-server/src/main/java/org/apache/zookeeper/server/NodeHashMapImpl.java

anmolnar · 2019-08-21T20:03:21Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/util/DigestCalculator.java

    }

+    private boolean digestEnabled;
+
+    public DigestCalculator() {


Move it to the same package as DataTree lives and make package-private everything.

@lvfangmin mentioned that he will use the DigestCalculator from other places, so I would wait for his PR. If it turns out still to be used only from this package, then I will move it.

He mentioned PrepRequestProcessor which is also in server package, so it should be even better there.

Let's make the class 'final' it will slightly help the JVM for optimizations.
We had 'static' methods that were not overridable.

This is a good idea. I tried it, but in SnapshotDigestTest.testDifferentDigestVersion we create a Mockito Spy on the class and apparently you can not create Mockito Spy on a final class.

So instead of this I take @anmolnar 's original idea and moved this class to the server package and made it packet private, so at least it can not be overwritten from non-ZK code. Let's hope it will work also for @lvfangmin. If not, then we can move back...

Besides PrepRequestProcessor, SerializeUtils will also reference to DigestCalculator when deserialize digest from txn:

https://github.com/apache/zookeeper/pull/1059/files#diff-d21faf00209dfd3c78b50788f07474ffR127.

@lvfangmin I think that DigestCalculator is an internal utility of DataTree and should not be exposed to anywhere else. PrepRequestProcessor could do better to access properties like digestEnabled and DIGEST_VERSION through DataTree getters. Like:

zks.getZKDatabase().getDataTree().getDigestEnabled();

It can cache the 2 values in the constructor like it's doing currently with digestEnabled.

Unfortunately SerializeUtils is a different story. It's another piece of logic which I think should be organised differently. It breaks both encapsulation and responsibility principles. Let me think about if there's a way to get round that.

… DataTree

symat · 2019-08-28T10:54:55Z

@lvfangmin

Also DigestCalculator is not only used in DataTree, it will also being used in PrepRequestProcessor when adding real time digest on txn, so this class is more about a util which could be replaced with different digest method. I'm preparing the PR in the last few days, will send the PR probably today.

Did you have time to send your PR? What do you think about the current proposal in general?

anmolnar

+1 LGTM.

anmolnar · 2019-08-28T11:42:48Z

@eolivelli @lvfangmin PTAL.

eolivelli

Overall is good.

I left 2 comments.

We need to know what @lvfangmin wants to do.

eolivelli · 2019-08-29T09:27:28Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/util/DigestCalculator.java

    }

+    private boolean digestEnabled;
+
+    public DigestCalculator() {


Let's make the class 'final' it will slightly help the JVM for optimizations.
We had 'static' methods that were not overridable.

eolivelli · 2019-08-29T09:29:16Z

zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java

@@ -219,7 +219,7 @@ public void testPzxidUpdatedWhenDeletingNonExistNode() throws Exception {
    @Test
    public void testDigestUpdatedWhenReplayCreateTxnForExistNode() {
        try {
-            DigestCalculator.setDigestEnabled(true);
+            System.setProperty(DigestCalculator.ZOOKEEPER_DIGEST_ENABLED, "true");


This won't have effect of you don't instantiate a new DataTree and the internal DigestCalculator

Nice catch, thanks! I modified the test to create a new DataTree after setting the system property.
(I also made the test cleaner by creating the DataTree now only in the test cases and removed the field from the setUp method - previously half the tests used the field, half of them not)

lvfangmin · 2019-08-29T18:17:26Z

@lvfangmin

Also DigestCalculator is not only used in DataTree, it will also being used in PrepRequestProcessor when adding real time digest on txn, so this class is more about a util which could be replaced with different digest method. I'm preparing the PR in the last few days, will send the PR probably today.

Did you have time to send your PR? What do you think about the current proposal in general?

@symat Sorry for the lately reply, the other PR has been sent out for review a week ago, you can take a look at here: #1059.

lvfangmin · 2019-08-29T19:16:04Z

This version with mockito.spy looks good to me, as long as we don't use things like setVersion.

The only downside of this is we'll need to pass the DigestCalculator around after changing it to non-static, probably I'll move the digestEnabled to some which could be shared across different classes.

lvfangmin · 2019-08-29T19:19:04Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/DigestCalculator.java

+    private boolean digestEnabled;
+
+    DigestCalculator() {
+        this.digestEnabled = Boolean.parseBoolean(System.getProperty(ZOOKEEPER_DIGEST_ENABLED, "true"));


Probably we should move this setting to ZooKeeperServer, so that it can be shared and used in static classes like SerializeUtil.

That could work.

anmolnar · 2019-09-04T08:45:52Z

@symat @eolivelli @lvfangmin Can we move on with this patch? I'd really like to see this test fixed and builds are green. Enrico you're still having -1 on it.

eolivelli

LGTM
but we should wait for @lvfangmin

lvfangmin

+1

Thanks @symat for making this work on JDK 12+, it seems better to move digestEnabled to ZooKeeperServer class.

symat · 2019-09-10T09:16:16Z

Thanks @lvfangmin for the comments!

I moved the digestEnabled to ZooKeeperServer class, and also created a setter for it. I saw that other configuration options are usually initialized by system properties, but then there are setters for them, which can also be used by the test code. So I followed this pattern.

In this way the digestEnabled option can be set from the code, while the digestVersion is still 'hidden' with private static final in DigestCalculator.

anmolnar · 2019-09-10T13:23:25Z

Merged to master. Thanks @symat !

The problem with the test `SnapshotDigestTest.testDifferentDigestVersion` was that it used reflection to change a final static value in `DigestCalculator`, what is no longer supported with JDK 12+ (see [JDK-8210522](https://bugs.openjdk.java.net/browse/JDK-8210522)) I think there are still some hacky solutions to go behind these limitations (with [PowerMock](https://github.com/powermock/powermock) maybe or using [VarHandle as suggested here](https://stackoverflow.com/questions/56039341/get-declared-fields-of-java-lang-reflect-fields-in-jdk12)), but I think the best is to avoid the situation when we need to poke the static final field. I changed the fully static `DigestCalculator`, converting it to a static singleton object with non-static methods / fields. The fields of the singleton object can not be modified from production code, but we can change them from the tests even in JDK 12 / 13 (as they are not static final fields). I tested it locally using openjdk, I hope it will also work in jenkins. Author: Mate Szalay-Beko <szalay.beko.mate@gmail.com> Author: Mate Szalay-Beko <mszalay@cloudera.com> Reviewers: fangmin@apache.org, eolivelli@apache.org, andor@apache.org Closes apache#1056 from symat/ZOOKEEPER-3495 and squashes the following commits: fa2a3b3 [Mate Szalay-Beko] Merge remote-tracking branch 'apache/master' into HEAD 67c5532 [Mate Szalay-Beko] ZOOKEEPER-3495 move digestEnabled to the ZooKeeperServer class c58a88c [Mate Szalay-Beko] ZOOKEEPER-3495: implement code-review comments 1a8cc2b [Mate Szalay-Beko] Merge remote-tracking branch 'origin/master' into ZOOKEEPER-3495 90da4c4 [Mate Szalay-Beko] ZOOKEEPER-3495: NodeHashMap use the same DigestCalculator instance as DataTree e1f659c [Mate Szalay-Beko] Merge remote-tracking branch 'origin/master' into ZOOKEEPER-3495 dbbf38f [Mate Szalay-Beko] ZOOKEEPER-3495: fix checkstyle errors 07741ae [Mate Szalay-Beko] Merge remote-tracking branch 'origin/master' into ZOOKEEPER-3495 d0d886d [Mate Szalay-Beko] ZOOKEEPER-3495: change implementation according to code review comments 2a80c32 [Mate Szalay-Beko] ZOOKEEPER-3495: fix SnapshotDigestTest to work with JDK12+

ZOOKEEPER-3495: fix SnapshotDigestTest to work with JDK12+

2a80c32

eolivelli requested changes Aug 15, 2019

View reviewed changes

anmolnar requested changes Aug 16, 2019

View reviewed changes

symat added 3 commits August 21, 2019 16:51

ZOOKEEPER-3495: change implementation according to code review comments

d0d886d

Merge remote-tracking branch 'origin/master' into ZOOKEEPER-3495

07741ae

ZOOKEEPER-3495: fix checkstyle errors

dbbf38f

anmolnar requested changes Aug 21, 2019

View reviewed changes

symat added 2 commits August 26, 2019 10:24

Merge remote-tracking branch 'origin/master' into ZOOKEEPER-3495

e1f659c

ZOOKEEPER-3495: NodeHashMap use the same DigestCalculator instance as…

90da4c4

… DataTree

anmolnar approved these changes Aug 28, 2019

View reviewed changes

anmolnar requested a review from eolivelli August 28, 2019 11:42

eolivelli requested changes Aug 29, 2019

View reviewed changes

symat added 2 commits August 29, 2019 19:04

Merge remote-tracking branch 'origin/master' into ZOOKEEPER-3495

1a8cc2b

ZOOKEEPER-3495: implement code-review comments

c58a88c

lvfangmin reviewed Aug 29, 2019

View reviewed changes

eolivelli approved these changes Sep 4, 2019

View reviewed changes

lvfangmin approved these changes Sep 10, 2019

View reviewed changes

Mate Szalay-Beko added 2 commits September 10, 2019 11:08

ZOOKEEPER-3495 move digestEnabled to the ZooKeeperServer class

67c5532

Merge remote-tracking branch 'apache/master' into HEAD

fa2a3b3

asfgit closed this in f01d01c Sep 10, 2019

ZOOKEEPER-3495: fix SnapshotDigestTest to work with JDK12+ #1056

ZOOKEEPER-3495: fix SnapshotDigestTest to work with JDK12+ #1056

Conversation

symat commented Aug 15, 2019

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli commented Aug 16, 2019

symat commented Aug 16, 2019

eolivelli commented Aug 16, 2019

anmolnar commented Aug 16, 2019

symat commented Aug 16, 2019

lvfangmin commented Aug 20, 2019

symat commented Aug 21, 2019 • edited

anmolnar commented Aug 21, 2019

anmolnar 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

symat commented Aug 28, 2019

anmolnar left a comment

Choose a reason for hiding this comment

anmolnar commented Aug 28, 2019

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

lvfangmin commented Aug 29, 2019

lvfangmin commented Aug 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolnar commented Sep 4, 2019

eolivelli left a comment

Choose a reason for hiding this comment

lvfangmin left a comment

Choose a reason for hiding this comment

symat commented Sep 10, 2019

anmolnar commented Sep 10, 2019

symat commented Aug 21, 2019 •

edited