> On May 4, 2016, 2 a.m., Jagadish Venkatraman wrote: > > samza-core/src/main/java/org/apache/samza/config/DefaultChooserConfig.java, > > line 36 > > <https://reviews.apache.org/r/46952/diff/1/?file=1370365#file1370365line36> > > > > any reason why this is not private?
no *good* reason :-) > On May 4, 2016, 2 a.m., Jagadish Venkatraman wrote: > > samza-core/src/main/java/org/apache/samza/config/DefaultChooserConfig.java, > > line 41 > > <https://reviews.apache.org/r/46952/diff/1/?file=1370365#file1370365line41> > > > > 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? >>Why do we need this method Config2DefaultChooser(config) if all it is doing >>is delegating to the underlying constructor? Mindless parity for the behavior of the scala implementation. Removed. > On May 4, 2016, 2 a.m., Jagadish Venkatraman wrote: > > samza-core/src/main/java/org/apache/samza/config/DefaultChooserConfig.java, > > line 59 > > <https://reviews.apache.org/r/46952/diff/1/?file=1370365#file1370365line59> > > > > 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..) I think I need to be more consisent with good ideas. :-) > On May 4, 2016, 2 a.m., Jagadish Venkatraman wrote: > > samza-core/src/main/java/org/apache/samza/config/DefaultChooserConfig.java, > > line 62 > > <https://reviews.apache.org/r/46952/diff/1/?file=1370365#file1370365line62> > > > > 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. More fallout of mindlessly copying the scala implementation. Thanks for raising it. > On May 4, 2016, 2 a.m., Jagadish Venkatraman wrote: > > samza-core/src/test/scala/org/apache/samza/system/chooser/TestDefaultChooser.scala, > > line 178 > > <https://reviews.apache.org/r/46952/diff/1/?file=1370369#file1370369line178> > > > > 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). See the next line, #169 - Jake ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46952/#review131607 ----------------------------------------------------------- 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 > >
