You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Jake Maes <ja...@gmail.com> on 2016/05/04 02:41:51 UTC
Review Request 46952: SAMZA-944 Broadcast stream is not added
properly in the prioritized tiers in the DefaultChooser
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46952/
-----------------------------------------------------------
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
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
Re: Review Request 46952: SAMZA-944 Broadcast stream is not added
properly in the prioritized tiers in the DefaultChooser
Posted by Jagadish Venkatraman <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46952/#review131633
-----------------------------------------------------------
Ship it!
Thanks for addressing the nits. LGTM. Ship It!
- Jagadish Venkatraman
On May 4, 2016, 3:34 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, 3:34 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
>
>
Re: Review Request 46952: SAMZA-944 Broadcast stream is not added
properly in the prioritized tiers in the DefaultChooser
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46952/#review131863
-----------------------------------------------------------
Ship it!
lgtm! +1
- Navina Ramesh
On May 4, 2016, 3:34 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, 3:34 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
>
>
Re: Review Request 46952: SAMZA-944 Broadcast stream is not added
properly in the prioritized tiers in the DefaultChooser
Posted by Jake Maes <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46952/
-----------------------------------------------------------
(Updated May 4, 2016, 3:34 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 (updated)
-----
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
Re: Review Request 46952: SAMZA-944 Broadcast stream is not added
properly in the prioritized tiers in the DefaultChooser
Posted by Jake Maes <ja...@gmail.com>.
> 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
>
>
Re: Review Request 46952: SAMZA-944 Broadcast stream is not added
properly in the prioritized tiers in the DefaultChooser
Posted by Jagadish Venkatraman <ja...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 46952: SAMZA-944 Broadcast stream is not added
properly in the prioritized tiers in the DefaultChooser
Posted by Jake Maes <ja...@gmail.com>.
-----------------------------------------------------------
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 (updated)
-------
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