----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32949/#review79397 -----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java <https://reviews.apache.org/r/32949/#comment128731> Can't this be final? exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java <https://reviews.apache.org/r/32949/#comment128707> Is it really right to return a new one each time, rather than finding and returning a single one (and creating it only the first time)? exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java <https://reviews.apache.org/r/32949/#comment128713> Can this be final? exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java <https://reviews.apache.org/r/32949/#comment128712> fail() should worry about doing something or ignoring calls according to the current state, rather than depending on the caller to worry about it. In this case, fail() would probably like to add the exception as a suppressed exception under a pre-existing fail, so it should still be called (subject to having any necessary adjustments to handle that). exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java <https://reviews.apache.org/r/32949/#comment128714> Can this be final? exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java <https://reviews.apache.org/r/32949/#comment128726> Does this really have to be an inner class? The only thing it references is FragmentContext.sendingAccountor. I would rather create an "AccountingDataTunnel" that takes the DataTunnel and a SendingAccountor as its constructor arguments. We might find other uses for that besides here. I'd rather keep FragmentContext as uncluttered as possible. exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java <https://reviews.apache.org/r/32949/#comment128715> final exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java <https://reviews.apache.org/r/32949/#comment128727> Same comment as FragmentDataTunnel re this being an inner class. This could be "AccountingUserConnection" or something like that. exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java <https://reviews.apache.org/r/32949/#comment128720> Too many contexts; can we please rename this to fragmentContext? Also, can't all three of these (stats, oContext, fragmentContext) be final? Remove the initializations to null, because it looks like they are all initialized in the constructors. exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java <https://reviews.apache.org/r/32949/#comment128721> Why is this (and incoming, and context) not private? exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java <https://reviews.apache.org/r/32949/#comment128722> Wouldn't it make more sense to call super.stop() after the logger message? After all, that is part of the stopping process. - Chris Westin On April 8, 2015, 12:10 p.m., Steven Phillips wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32949/ > ----------------------------------------------------------- > > (Updated April 8, 2015, 12:10 p.m.) > > > Review request for drill, Chris Westin and Jacques Nadeau. > > > Repository: drill-git > > > Description > ------- > > Creates wrapper classes FragmentDataTunnel and FragmentUserDataTunnel which > wrap > the DataTunnel and UserClientConnection, respectively, allowing us to use > DataTunnels > and UserClientConnections from a global pool, but track pending batches and > send status > at the FragmentContext level. > > Consolidates the various StatusListener implementations used by the various > senders and > instead uses just one implementation. > > > Diffs > ----- > > exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java > 18b93e9 > > exec/java-exec/src/main/java/org/apache/drill/exec/ops/SendingAccountor.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java > a00df9d > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java > 8038527 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java > 21fc800 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SingleSenderCreator.java > 1ef7bbd > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java > d17fdd4 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java > 6a73cdd > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java > 9d6e98f > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java > 33d6f95 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/StatusHandler.java > 5e21878 > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java > 8022c95 > > Diff: https://reviews.apache.org/r/32949/diff/ > > > Testing > ------- > > No new functionality, so no new tests. Current tests all pass. > > > Thanks, > > Steven Phillips > >
