KAFKA-3502: move RocksDB options construction to init() by guozhangwang · Pull Request #2381 · apache/kafka

guozhangwang · 2017-01-15T06:29:00Z

In RocksDBStore, options / wOptions / fOptions are constructed in the constructor, which needs to be dismissed in the close() call; however in some tests, the generated topology is not initialized at all, and hence the corresponding state stores are supposed to not be able to be closed as well since their init function is not called. This could cause the above option objects to be not released.

This is fixed in this patch to move the logic out of constructor and inside init functions, so that no RocksDB objects will be created in the constructor only. Also some minor cleanups:

In KStreamTestDriver.close(), we lost the logic to close the state stores but only call flush; it is now changed back to call both.
Moved the forwarding logic from KStreamTestDriver to MockProcessorContext to remove the mutual dependency: these functions should really be in ProcessorContext, not the test driver.

guozhangwang · 2017-01-15T06:29:13Z

ping @enothereska @dguy @mjsax for reviews.

asfbot · 2017-01-15T07:22:21Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/888/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-01-15T07:22:37Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/890/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-01-15T07:27:11Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/888/
Test FAILed (JDK 7 and Scala 2.10).

mjsax

LGTM

mjsax

This fix seems to be incomplete:

org.apache.kafka.streams.state.internals.SegmentIteratorTest > shouldOnlyIterateOverSegmentsInRange PASSED
pure virtual method called
terminate called without an active exception

guozhangwang · 2017-01-16T01:50:48Z

This fix seems to be incomplete:

Looking into it.

guozhangwang · 2017-01-16T04:16:12Z

Found another issue with SegmentIteratorTest, where the test could be opening the underlying Segment's iterator without closing it at the end if it is not calling hasNext until it returns false.

asfbot · 2017-01-16T05:03:08Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/899/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-01-16T05:04:06Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/897/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-01-16T05:10:40Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/897/
Test PASSed (JDK 7 and Scala 2.10).

dguy

1 minor comment, otherwise LGTM

dguy · 2017-01-16T10:26:47Z

+    private static final String APP_ID = "app-id";
+
    private KStreamTestDriver driver = null;
+    private KStreamBuilder builder = null;


The KStreamBuilder could be created here. Can also be final

…into K3502-pure-virtual-function-unit-tests

asfbot · 2017-01-16T20:31:04Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/919/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-01-16T20:34:27Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/921/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-01-16T22:45:55Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/919/
Test FAILed (JDK 8 and Scala 2.12).

mjsax

LGTM

guozhangwang · 2017-01-17T20:58:44Z

retest this please

asfbot · 2017-01-17T21:03:04Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/945/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-01-17T21:49:03Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/943/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-01-17T21:50:42Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/943/
Test PASSed (JDK 8 and Scala 2.12).

hachikuji · 2017-01-17T22:23:37Z

-        final MockProcessorContext context = new MockProcessorContext(null,
-                                                                      stateDir,
+        final MockProcessorContext context = new MockProcessorContext(
+                stateDir,


nit: the alignment got messed up on a bunch of these.

hachikuji · 2017-01-17T22:40:11Z

+        return topology;
+    }
+
+    public ProcessorTopology globalTopology() {


nit: seems this is unused?

hachikuji · 2017-01-17T23:00:17Z

@guozhangwang Other than a couple nitpicks, LGTM.

asfbot · 2017-01-18T02:08:02Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/969/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-01-18T02:08:21Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/971/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-01-18T02:10:50Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/969/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-01-18T03:11:49Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/972/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-01-18T03:13:10Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/970/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-01-18T04:39:33Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/970/
Test FAILed (JDK 7 and Scala 2.10).

In RocksDBStore, options / wOptions / fOptions are constructed in the constructor, which needs to be dismissed in the close() call; however in some tests, the generated topology is not initialized at all, and hence the corresponding state stores are supposed to not be able to be closed as well since their `init` function is not called. This could cause the above option objects to be not released. This is fixed in this patch to move the logic out of constructor and inside `init` functions, so that no RocksDB objects will be created in the constructor only. Also some minor cleanups: 1. In KStreamTestDriver.close(), we lost the logic to close the state stores but only call `flush`; it is now changed back to call both. 2. Moved the forwarding logic from KStreamTestDriver to MockProcessorContext to remove the mutual dependency: these functions should really be in ProcessorContext, not the test driver. Author: Guozhang Wang <wangguoz@gmail.com> Reviewers: Damian Guy <damian.guy@gmail.com>, Matthias J. Sax <matthias@confluent.io>, Jason Gustafson <jason@confluent.io> Closes #2381 from guozhangwang/K3502-pure-virtual-function-unit-tests (cherry picked from commit 1974e1b) Signed-off-by: Jason Gustafson <jason@confluent.io>

hachikuji · 2017-01-18T04:41:06Z

Thanks for the patch! Fixed one more alignment nit when merging.

In RocksDBStore, options / wOptions / fOptions are constructed in the constructor, which needs to be dismissed in the close() call; however in some tests, the generated topology is not initialized at all, and hence the corresponding state stores are supposed to not be able to be closed as well since their `init` function is not called. This could cause the above option objects to be not released. This is fixed in this patch to move the logic out of constructor and inside `init` functions, so that no RocksDB objects will be created in the constructor only. Also some minor cleanups: 1. In KStreamTestDriver.close(), we lost the logic to close the state stores but only call `flush`; it is now changed back to call both. 2. Moved the forwarding logic from KStreamTestDriver to MockProcessorContext to remove the mutual dependency: these functions should really be in ProcessorContext, not the test driver. Author: Guozhang Wang <wangguoz@gmail.com> Reviewers: Damian Guy <damian.guy@gmail.com>, Matthias J. Sax <matthias@confluent.io>, Jason Gustafson <jason@confluent.io> Closes apache#2381 from guozhangwang/K3502-pure-virtual-function-unit-tests

move options construction to init()

78870f7

mjsax approved these changes Jan 15, 2017

View reviewed changes

mjsax reviewed Jan 15, 2017

View reviewed changes

fix SegmentIteratorTest

28449dd

dguy reviewed Jan 16, 2017

View reviewed changes

guozhangwang added 2 commits January 16, 2017 11:42

Merge branch 'trunk' of https://git-wip-us.apache.org/repos/asf/kafka …

4411dfd

…into K3502-pure-virtual-function-unit-tests

github comments

dc6ffe0

dguy approved these changes Jan 17, 2017

View reviewed changes

mjsax approved these changes Jan 17, 2017

View reviewed changes

hachikuji reviewed Jan 17, 2017

View reviewed changes

guozhangwang added 2 commits January 17, 2017 18:01

rebase from apache trunk

7bca9d6

github comments

97c51f7

fix unit tests

2dcfebd

asfgit closed this in 1974e1b Jan 18, 2017

guozhangwang deleted the K3502-pure-virtual-function-unit-tests branch July 15, 2017 22:07

Conversation

guozhangwang commented Jan 15, 2017

Uh oh!

guozhangwang commented Jan 15, 2017

Uh oh!

asfbot commented Jan 15, 2017

Uh oh!

asfbot commented Jan 15, 2017

Uh oh!

asfbot commented Jan 15, 2017

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Jan 16, 2017

Uh oh!

guozhangwang commented Jan 16, 2017

Uh oh!

asfbot commented Jan 16, 2017

Uh oh!

asfbot commented Jan 16, 2017

Uh oh!

asfbot commented Jan 16, 2017

Uh oh!

dguy left a comment

Choose a reason for hiding this comment

Uh oh!

dguy Jan 16, 2017

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jan 16, 2017

Uh oh!

asfbot commented Jan 16, 2017

Uh oh!

asfbot commented Jan 16, 2017

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Jan 17, 2017

Uh oh!

asfbot commented Jan 17, 2017

Uh oh!

asfbot commented Jan 17, 2017

Uh oh!

asfbot commented Jan 17, 2017

Uh oh!

hachikuji Jan 17, 2017

Choose a reason for hiding this comment

Uh oh!

hachikuji Jan 17, 2017

Choose a reason for hiding this comment

Uh oh!

hachikuji commented Jan 17, 2017

Uh oh!

asfbot commented Jan 18, 2017

Uh oh!

asfbot commented Jan 18, 2017

Uh oh!

asfbot commented Jan 18, 2017

Uh oh!

asfbot commented Jan 18, 2017

Uh oh!

asfbot commented Jan 18, 2017

Uh oh!

asfbot commented Jan 18, 2017

Uh oh!

hachikuji commented Jan 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants