You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Vishal Kuo <vi...@gmail.com> on 2016/04/16 18:22:40 UTC
Review Request 46303: SAMZA-935: add functionality to wait for kafka
to start
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46303/
-----------------------------------------------------------
Review request for samza.
Repository: samza-hello-samza
Description
-------
To prevent kafka dependent services and unit tests to fail intermittently after bootstrap, a wait_kafka function has been added
Diffs
-----
bin/grid 042cabe355ec3a8fff06f193252fec494d01a864
Diff: https://reviews.apache.org/r/46303/diff/
Testing
-------
* Tested normal bootstrap procedure works with additional wait added for kafka
* Tested that timeout occurs if Kafka takes too long to start
Thanks,
Vishal Kuo
Re: Review Request 46303: SAMZA-935: add functionality to wait for
kafka to start
Posted by Vishal Kuo <vi...@gmail.com>.
> On April 18, 2016, 4:49 p.m., Navina Ramesh wrote:
> > bin/grid, line 130
> > <https://reviews.apache.org/r/46303/diff/2/?file=1348500#file1348500line130>
> >
> > Ah.. Can you add the wait_for_service line here for zookeeper?
> > wait_for_service "zookeeper" 2181
Done
> On April 18, 2016, 4:49 p.m., Navina Ramesh wrote:
> > bin/grid, line 140
> > <https://reviews.apache.org/r/46303/diff/2/?file=1348500#file1348500line140>
> >
> > Can you add the wait_for_service line here for RM and NM?
> > wait_for_service "resourcemanager" 8032
> > wait_for_service "nodemanager" 8042
Done
- Vishal
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46303/#review129365
-----------------------------------------------------------
On April 18, 2016, 6:28 p.m., Vishal Kuo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46303/
> -----------------------------------------------------------
>
> (Updated April 18, 2016, 6:28 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza-hello-samza
>
>
> Description
> -------
>
> To prevent kafka dependent services and unit tests to fail intermittently after bootstrap, a wait_kafka function has been added
>
>
> Diffs
> -----
>
> bin/grid 042cabe355ec3a8fff06f193252fec494d01a864
>
> Diff: https://reviews.apache.org/r/46303/diff/
>
>
> Testing
> -------
>
> * Tested normal bootstrap procedure works with additional wait added for kafka
> * Tested that timeout occurs if Kafka takes too long to start
>
>
> Thanks,
>
> Vishal Kuo
>
>
Re: Review Request 46303: SAMZA-935: add functionality to wait for
kafka to start
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46303/#review129365
-----------------------------------------------------------
bin/grid (line 130)
<https://reviews.apache.org/r/46303/#comment192804>
Ah.. Can you add the wait_for_service line here for zookeeper?
wait_for_service "zookeeper" 2181
bin/grid (line 140)
<https://reviews.apache.org/r/46303/#comment192805>
Can you add the wait_for_service line here for RM and NM?
wait_for_service "resourcemanager" 8032
wait_for_service "nodemanager" 8042
- Navina Ramesh
On April 18, 2016, 3:29 p.m., Vishal Kuo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46303/
> -----------------------------------------------------------
>
> (Updated April 18, 2016, 3:29 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza-hello-samza
>
>
> Description
> -------
>
> To prevent kafka dependent services and unit tests to fail intermittently after bootstrap, a wait_kafka function has been added
>
>
> Diffs
> -----
>
> bin/grid 042cabe355ec3a8fff06f193252fec494d01a864
>
> Diff: https://reviews.apache.org/r/46303/diff/
>
>
> Testing
> -------
>
> * Tested normal bootstrap procedure works with additional wait added for kafka
> * Tested that timeout occurs if Kafka takes too long to start
>
>
> Thanks,
>
> Vishal Kuo
>
>
Re: Review Request 46303: SAMZA-935: add functionality to wait for
kafka to start
Posted by Vishal Kuo <vi...@gmail.com>.
> On April 19, 2016, 6:23 p.m., Boris Shkolnik wrote:
> > bin/grid, line 165
> > <https://reviews.apache.org/r/46303/diff/3/?file=1348609#file1348609line165>
> >
> > just wondering - why -w 2? -w 1 should work as well? If the port is not open it will return right away, will it?
Sorry about that, the 2 was just arbitrary. 1 makes more sense.
> On April 19, 2016, 6:23 p.m., Boris Shkolnik wrote:
> > bin/grid, line 41
> > <https://reviews.apache.org/r/46303/diff/3/?file=1348609#file1348609line41>
> >
> > can you please change the name to specify the time units? (.._SEC). It makes it clearer.
All changes made
- Vishal
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46303/#review129584
-----------------------------------------------------------
On April 19, 2016, 7:39 p.m., Vishal Kuo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46303/
> -----------------------------------------------------------
>
> (Updated April 19, 2016, 7:39 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza-hello-samza
>
>
> Description
> -------
>
> To prevent kafka dependent services and unit tests to fail intermittently after bootstrap, a wait_kafka function has been added
>
>
> Diffs
> -----
>
> bin/grid 042cabe355ec3a8fff06f193252fec494d01a864
>
> Diff: https://reviews.apache.org/r/46303/diff/
>
>
> Testing
> -------
>
> * Tested normal bootstrap procedure works with additional wait added for kafka
> * Tested that timeout occurs if Kafka takes too long to start
>
>
> Thanks,
>
> Vishal Kuo
>
>
Re: Review Request 46303: SAMZA-935: add functionality to wait for
kafka to start
Posted by Boris Shkolnik <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46303/#review129584
-----------------------------------------------------------
bin/grid (line 41)
<https://reviews.apache.org/r/46303/#comment193018>
can you please change the name to specify the time units? (.._SEC). It makes it clearer.
bin/grid (line 129)
<https://reviews.apache.org/r/46303/#comment193014>
nit. please remove the space.
bin/grid (line 130)
<https://reviews.apache.org/r/46303/#comment193020>
Could you please use GLOBAL_VAR for the port numbers and declare them upfront (in case someone ever uses it for different purpose).
bin/grid (line 161)
<https://reviews.apache.org/r/46303/#comment193017>
please make the variables local(to avoid overwriting a global ones).
bin/grid (line 165)
<https://reviews.apache.org/r/46303/#comment193031>
just wondering - why -w 2? -w 1 should work as well? If the port is not open it will return right away, will it?
- Boris Shkolnik
On April 18, 2016, 6:28 p.m., Vishal Kuo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46303/
> -----------------------------------------------------------
>
> (Updated April 18, 2016, 6:28 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza-hello-samza
>
>
> Description
> -------
>
> To prevent kafka dependent services and unit tests to fail intermittently after bootstrap, a wait_kafka function has been added
>
>
> Diffs
> -----
>
> bin/grid 042cabe355ec3a8fff06f193252fec494d01a864
>
> Diff: https://reviews.apache.org/r/46303/diff/
>
>
> Testing
> -------
>
> * Tested normal bootstrap procedure works with additional wait added for kafka
> * Tested that timeout occurs if Kafka takes too long to start
>
>
> Thanks,
>
> Vishal Kuo
>
>
Re: Review Request 46303: SAMZA-935: add functionality to wait for
kafka to start
Posted by Vishal Kuo <vi...@gmail.com>.
> On April 19, 2016, 12:37 a.m., Vishal Kuo wrote:
> > Ship It!
This was accidental
- Vishal
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46303/#review129458
-----------------------------------------------------------
On April 19, 2016, 7:39 p.m., Vishal Kuo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46303/
> -----------------------------------------------------------
>
> (Updated April 19, 2016, 7:39 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza-hello-samza
>
>
> Description
> -------
>
> To prevent kafka dependent services and unit tests to fail intermittently after bootstrap, a wait_kafka function has been added
>
>
> Diffs
> -----
>
> bin/grid 042cabe355ec3a8fff06f193252fec494d01a864
>
> Diff: https://reviews.apache.org/r/46303/diff/
>
>
> Testing
> -------
>
> * Tested normal bootstrap procedure works with additional wait added for kafka
> * Tested that timeout occurs if Kafka takes too long to start
>
>
> Thanks,
>
> Vishal Kuo
>
>
Re: Review Request 46303: SAMZA-935: add functionality to wait for
kafka to start
Posted by Vishal Kuo <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46303/#review129458
-----------------------------------------------------------
Ship it!
Ship It!
- Vishal Kuo
On April 18, 2016, 6:28 p.m., Vishal Kuo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46303/
> -----------------------------------------------------------
>
> (Updated April 18, 2016, 6:28 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza-hello-samza
>
>
> Description
> -------
>
> To prevent kafka dependent services and unit tests to fail intermittently after bootstrap, a wait_kafka function has been added
>
>
> Diffs
> -----
>
> bin/grid 042cabe355ec3a8fff06f193252fec494d01a864
>
> Diff: https://reviews.apache.org/r/46303/diff/
>
>
> Testing
> -------
>
> * Tested normal bootstrap procedure works with additional wait added for kafka
> * Tested that timeout occurs if Kafka takes too long to start
>
>
> Thanks,
>
> Vishal Kuo
>
>
Re: Review Request 46303: SAMZA-935: add functionality to wait for
kafka to start
Posted by Boris Shkolnik <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46303/#review129603
-----------------------------------------------------------
Ship it!
Lgtm! Thanks!
- Boris Shkolnik
On April 19, 2016, 7:39 p.m., Vishal Kuo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46303/
> -----------------------------------------------------------
>
> (Updated April 19, 2016, 7:39 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza-hello-samza
>
>
> Description
> -------
>
> To prevent kafka dependent services and unit tests to fail intermittently after bootstrap, a wait_kafka function has been added
>
>
> Diffs
> -----
>
> bin/grid 042cabe355ec3a8fff06f193252fec494d01a864
>
> Diff: https://reviews.apache.org/r/46303/diff/
>
>
> Testing
> -------
>
> * Tested normal bootstrap procedure works with additional wait added for kafka
> * Tested that timeout occurs if Kafka takes too long to start
>
>
> Thanks,
>
> Vishal Kuo
>
>
Re: Review Request 46303: SAMZA-935: add functionality to wait for
kafka to start
Posted by Vishal Kuo <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46303/
-----------------------------------------------------------
(Updated April 19, 2016, 7:39 p.m.)
Review request for samza.
Changes
-------
Extracted ports to global variables, made function variables local, changed nc -w 2 to nc -w 1
Repository: samza-hello-samza
Description
-------
To prevent kafka dependent services and unit tests to fail intermittently after bootstrap, a wait_kafka function has been added
Diffs (updated)
-----
bin/grid 042cabe355ec3a8fff06f193252fec494d01a864
Diff: https://reviews.apache.org/r/46303/diff/
Testing
-------
* Tested normal bootstrap procedure works with additional wait added for kafka
* Tested that timeout occurs if Kafka takes too long to start
Thanks,
Vishal Kuo
Re: Review Request 46303: SAMZA-935: add functionality to wait for
kafka to start
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46303/#review129419
-----------------------------------------------------------
Ship it!
Looks good! Thanks for the changes. +1
- Navina Ramesh
On April 18, 2016, 6:28 p.m., Vishal Kuo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46303/
> -----------------------------------------------------------
>
> (Updated April 18, 2016, 6:28 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza-hello-samza
>
>
> Description
> -------
>
> To prevent kafka dependent services and unit tests to fail intermittently after bootstrap, a wait_kafka function has been added
>
>
> Diffs
> -----
>
> bin/grid 042cabe355ec3a8fff06f193252fec494d01a864
>
> Diff: https://reviews.apache.org/r/46303/diff/
>
>
> Testing
> -------
>
> * Tested normal bootstrap procedure works with additional wait added for kafka
> * Tested that timeout occurs if Kafka takes too long to start
>
>
> Thanks,
>
> Vishal Kuo
>
>
Re: Review Request 46303: SAMZA-935: add functionality to wait for
kafka to start
Posted by Vishal Kuo <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46303/
-----------------------------------------------------------
(Updated April 18, 2016, 6:28 p.m.)
Review request for samza.
Changes
-------
Added service wait to zookeeper, namenode, and resourcemanager
Repository: samza-hello-samza
Description
-------
To prevent kafka dependent services and unit tests to fail intermittently after bootstrap, a wait_kafka function has been added
Diffs (updated)
-----
bin/grid 042cabe355ec3a8fff06f193252fec494d01a864
Diff: https://reviews.apache.org/r/46303/diff/
Testing
-------
* Tested normal bootstrap procedure works with additional wait added for kafka
* Tested that timeout occurs if Kafka takes too long to start
Thanks,
Vishal Kuo
Re: Review Request 46303: SAMZA-935: add functionality to wait for
kafka to start
Posted by Vishal Kuo <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46303/
-----------------------------------------------------------
(Updated April 18, 2016, 3:29 p.m.)
Review request for samza.
Changes
-------
Changed wait_kafka function to generic wait_for_service function
Repository: samza-hello-samza
Description
-------
To prevent kafka dependent services and unit tests to fail intermittently after bootstrap, a wait_kafka function has been added
Diffs (updated)
-----
bin/grid 042cabe355ec3a8fff06f193252fec494d01a864
Diff: https://reviews.apache.org/r/46303/diff/
Testing
-------
* Tested normal bootstrap procedure works with additional wait added for kafka
* Tested that timeout occurs if Kafka takes too long to start
Thanks,
Vishal Kuo
Re: Review Request 46303: SAMZA-935: add functionality to wait for
kafka to start
Posted by Vishal Kuo <vi...@gmail.com>.
On April 18, 2016, 5:34 a.m., Vishal Kuo wrote:
> > Changes look good except for one minor improvement comment. Also, how does this fix help prevent "unit test failures" as you have mentioned in the RB's description?
Hi Navina,
Thanks for the review, the "unit test failures" portion was a mistake on my part, the dependent services failing was the main issue I wanted to address.
I've made the suggested changes.
- Vishal
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46303/#review129296
-----------------------------------------------------------
On April 16, 2016, 4:22 p.m., Vishal Kuo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46303/
> -----------------------------------------------------------
>
> (Updated April 16, 2016, 4:22 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza-hello-samza
>
>
> Description
> -------
>
> To prevent kafka dependent services and unit tests to fail intermittently after bootstrap, a wait_kafka function has been added
>
>
> Diffs
> -----
>
> bin/grid 042cabe355ec3a8fff06f193252fec494d01a864
>
> Diff: https://reviews.apache.org/r/46303/diff/
>
>
> Testing
> -------
>
> * Tested normal bootstrap procedure works with additional wait added for kafka
> * Tested that timeout occurs if Kafka takes too long to start
>
>
> Thanks,
>
> Vishal Kuo
>
>
Re: Review Request 46303: SAMZA-935: add functionality to wait for
kafka to start
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46303/#review129296
-----------------------------------------------------------
bin/grid (line 157)
<https://reviews.apache.org/r/46303/#comment192768>
This makes our bootstrap script more robust. I think it will be valuable to make this generic as:
wait_for_service(service_name, port_number)
Changes look good except for one minor improvement comment. Also, how does this fix help prevent "unit test failures" as you have mentioned in the RB's description?
- Navina Ramesh
On April 16, 2016, 4:22 p.m., Vishal Kuo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46303/
> -----------------------------------------------------------
>
> (Updated April 16, 2016, 4:22 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza-hello-samza
>
>
> Description
> -------
>
> To prevent kafka dependent services and unit tests to fail intermittently after bootstrap, a wait_kafka function has been added
>
>
> Diffs
> -----
>
> bin/grid 042cabe355ec3a8fff06f193252fec494d01a864
>
> Diff: https://reviews.apache.org/r/46303/diff/
>
>
> Testing
> -------
>
> * Tested normal bootstrap procedure works with additional wait added for kafka
> * Tested that timeout occurs if Kafka takes too long to start
>
>
> Thanks,
>
> Vishal Kuo
>
>