Adar Dembo has posted comments on this change. Change subject: Fix Spark shutdown ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/2571/2//COMMIT_MSG Commit Message: Line 10: the Spark shell being unable to close properly. This commit also adds a shutdown Hmm, I don't know about this. I find shutdown hooks to be pretty hack-ish; better to get the behavior right (i.e. ensure all threads are daemon or require clients to invoke a close() method that shuts down the thread pools) instead. http://gerrit.cloudera.org:8080/#/c/2571/2/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java: Line 201: private final HashedWheelTimer timer; FWIW, I don't think it's important to use the same timer for our own timeouts as for the channel factory, as AsyncKuduClient.shutdown() explicitly calls timer.stop() which (should?) kill all the threads in the timer. Line 235: this.channelFactory = b.createChannelFactory(); Not your fault, but do you know why the AsyncKuduClient is invoking a method on the builder to get the channel factory? It's strange that builder.build(), as part of creating the AsyncKuduClient, calls back into itself (via createChannelFactory()). I'd expect build() to create the channel factory, store it in a builder field, then create the AsyncKuduClient, which would reference the builder's field directly. The current code is counter-intuitive, but maybe there's a good reason to do it this way that I'm not seeing. Line 1992: new HashedWheelTimer(new ThreadFactoryBuilder().setDaemon(true).build(), 20, MILLISECONDS); The default is 100 ms; it would be nice to figure out why we're using 20 and add a comment. Line 2127: new NioWorkerPool(worker, workerCount), This is just a side-effect of using the constructor with a customized timer, right? Could you add a comment to that effect? Line 2128: timer); So why do we need to use a timer with daemon threads? AsyncKuduClient.shutdown() calls channelFactory.releaseExternalResources(); isn't that insufficient to kill the timer's threads? Or is the issue that shutdown() isn't being called at all? http://gerrit.cloudera.org:8080/#/c/2571/2/java/kudu-spark/src/main/scala/org/kududb/spark/KuduContext.scala File java/kudu-spark/src/main/scala/org/kududb/spark/KuduContext.scala: Line 38: println("Creating and registering sync client") Was this just for debugging? -- To view, visit http://gerrit.cloudera.org:8080/2571 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I30a64ec5eb30d70361204646523c9947d88c251f Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
