----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46952/#review131607 -----------------------------------------------------------
samza-core/src/main/java/org/apache/samza/config/DefaultChooserConfig.java (line 36) <https://reviews.apache.org/r/46952/#comment195619> any reason why this is not private? samza-core/src/main/java/org/apache/samza/config/DefaultChooserConfig.java (line 41) <https://reviews.apache.org/r/46952/#comment195624> I see there are 2 ways of instantiating a DefaultChooserConfig.. 1. Using the public factory method 2. Using the public constructor If we want instantiations done by the factory method, would n't it be nice to make the constructor private? Why do we need this method Config2DefaultChooser(config) if all it is doing is delegating to the underlying constructor? samza-core/src/main/java/org/apache/samza/config/DefaultChooserConfig.java (line 59) <https://reviews.apache.org/r/46952/#comment195626> What do you think about returning an unmodifiable view of the set ? That way we can say that this is read only? (I like that you've returned an immutable data structure in TaskConfigJava..) samza-core/src/main/java/org/apache/samza/config/DefaultChooserConfig.java (line 62) <https://reviews.apache.org/r/46952/#comment195620> nit: would be useful to add javadocs to the method. It maybe obvious for getchooserbatchsize() and getbootstrapstreams(). IMO, Some docs for getPriorityStreams() will be useful. Also, it may be good to document what the structure of the map returned is? I believe it is <ssp, priority>, but it would be good to call it out. samza-core/src/main/java/org/apache/samza/config/DefaultChooserConfig.java (line 72) <https://reviews.apache.org/r/46952/#comment195625> What do you think about returning an unmodifiable map? That way we have guarantees that people can't arbitrarily mutate it outside? samza-core/src/main/java/org/apache/samza/config/TaskConfigJava.java (line 107) <https://reviews.apache.org/r/46952/#comment195621> nit: todo testme formatting. samza-core/src/test/scala/org/apache/samza/system/chooser/TestDefaultChooser.scala (line 168) <https://reviews.apache.org/r/46952/#comment195623> What do you think about adding an assert for the priority-value too? (just to verify it's indeed 3). That way we can make the test robust if some logic in getPriorityStreams changes. (currently, we only put values in the map if it's >0). Overall, the code looks good to me. Apart from minor nits. - Jagadish Venkatraman On May 4, 2016, 12:42 a.m., Jake Maes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46952/ > ----------------------------------------------------------- > > (Updated May 4, 2016, 12:42 a.m.) > > > Review request for samza, Boris Shkolnik, Chris Pettitt, Navina Ramesh, > Jagadish Venkatraman, and Yi Pan (Data Infrastructure). > > > Bugs: SAMZA-944 > https://issues.apache.org/jira/browse/SAMZA-944 > > > Repository: samza > > > Description > ------- > > SAMZA-944 Broadcast stream is not added properly in the prioritized tiers in > the DefaultChooser > > * DefaultChooserConfig has been converted to Java. It also now takes ALL > input streams (including broadcast streams) into account when returning > bootstrap and prioritized streams > * TaskConfigJava - getBroadcastSystemStreamPartitions can now be called even > if there are no configured broadcast streams. It also has a new method to > return ALL input systems, including broadcast streams. > * DefaultChooser.java - only changed to handle conversion between java/scala > data structures, sigh > > > Diffs > ----- > > checkstyle/import-control.xml c15b8e74de8e5aac5ac83278c52ab3dba1630e50 > samza-core/src/main/java/org/apache/samza/config/DefaultChooserConfig.java > PRE-CREATION > samza-core/src/main/java/org/apache/samza/config/TaskConfigJava.java > 8acb6ca1cd220e715272d254398545ce487c0ff7 > > samza-core/src/main/scala/org/apache/samza/config/DefaultChooserConfig.scala > 422439355565616d7b629f2fcbc9207d709ef78c > > samza-core/src/main/scala/org/apache/samza/system/chooser/DefaultChooser.scala > 95bd18898dd9e2b6848523fe89c9017a7267ab3b > > samza-core/src/test/scala/org/apache/samza/system/chooser/TestDefaultChooser.scala > 090995653a6314e081f14729ea092d61e89c1a86 > > Diff: https://reviews.apache.org/r/46952/diff/ > > > Testing > ------- > > 2 new unit tests and ran check_all.sh > > > Thanks, > > Jake Maes > >
