Re: Review Request 36545: SAMZA-682 Refactor Coordinator stream messages
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/#review93826 --- Ship it! Thanks for testing with Zopkio ! - Navina Ramesh On July 31, 2015, 12:49 p.m., József Márton Jung wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/ --- (Updated July 31, 2015, 12:49 p.m.) Review request for samza. Repository: samza Description --- The following has been refactored: 1. Static inner classes from CoordinatorStreamMessage has been extracted 2. Common functionality from CheckpointManager, ChangelogMappingManager and LocalityManager has benn moved to a base class Diffs - checkstyle/import-control.xml 6654319 samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java 7445996 samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 55c258f samza-core/src/main/java/org/apache/samza/coordinator/stream/AbstractCoordinatorStreamManager.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamMessage.java e5ab4fb samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java b1078bd samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemProducer.java 92f8907 samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamWriter.java f769756 samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/CoordinatorStreamMessage.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/Delete.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetChangelogMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetCheckpoint.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetConfig.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetContainerHostMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/job/model/JobModel.java ad6387d samza-core/src/main/java/org/apache/samza/storage/ChangelogPartitionManager.java 7d3409c samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 0dbf14b samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 1c178a6 samza-core/src/test/java/org/apache/samza/coordinator/stream/MockCoordinatorStreamWrappedConsumer.java e454593 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamMessage.java ac26a01 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemConsumer.java c25f6a7 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemProducer.java 1ef07d0 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamWriter.java c484660 samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala 84fdeaa samza-yarn/src/main/resources/scalate/WEB-INF/views/index.scaml 41303f7 Diff: https://reviews.apache.org/r/36545/diff/ Testing --- Tests has been updated. Thanks, József Márton Jung
Re: Review Request 36545: SAMZA-682 Refactor Coordinator stream messages
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/ --- (Updated July 31, 2015, 12:49 p.m.) Review request for samza. Changes --- Updated the patch, it can be cleanly applied to current master branch. Comments addressed. Repository: samza Description --- The following has been refactored: 1. Static inner classes from CoordinatorStreamMessage has been extracted 2. Common functionality from CheckpointManager, ChangelogMappingManager and LocalityManager has benn moved to a base class Diffs (updated) - checkstyle/import-control.xml 6654319 samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java 7445996 samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 55c258f samza-core/src/main/java/org/apache/samza/coordinator/stream/AbstractCoordinatorStreamManager.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamMessage.java e5ab4fb samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java b1078bd samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemProducer.java 92f8907 samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamWriter.java f769756 samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/CoordinatorStreamMessage.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/Delete.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetChangelogMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetCheckpoint.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetConfig.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetContainerHostMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/job/model/JobModel.java ad6387d samza-core/src/main/java/org/apache/samza/storage/ChangelogPartitionManager.java 7d3409c samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 0dbf14b samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 1c178a6 samza-core/src/test/java/org/apache/samza/coordinator/stream/MockCoordinatorStreamWrappedConsumer.java e454593 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamMessage.java ac26a01 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemConsumer.java c25f6a7 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemProducer.java 1ef07d0 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamWriter.java c484660 samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala 84fdeaa samza-yarn/src/main/resources/scalate/WEB-INF/views/index.scaml 41303f7 Diff: https://reviews.apache.org/r/36545/diff/ Testing --- Tests has been updated. Thanks, József Márton Jung
Re: Review Request 36545: SAMZA-682 Refactor Coordinator stream messages
On July 24, 2015, 6:01 p.m., Navina Ramesh wrote: Thanks for picking this up! It feels good to look at a refactored code. One suggestion: Please run all the intergration test (including the zopkio tests) before checking in this patch. I don't think we cleanly start and stop coordinator stream producers/consumers in all the managers. Please verify that nothing is broken due to this change. József Márton Jung wrote: I have difficulties running the zopkio tests. The error message is the following: 2015-07-27 11:36:44,608 zopkio.remote_host_helper [ERROR] Error: JAVA_HOME is not set and could not be found. JAVA_HOME is set on my machine (in /etc/profile, so it is available system-wide), echoing it outputs the path to Java installation. I don't knw what is wrong. Navina Ramesh wrote: I think the problem is with the remote host (host to which zopkio is trying to ssh) not having JAVA_HOME set correctly. Are you running the test on a remote machine? József Márton Jung wrote: I'm trying to run the tests locally, so zopkio is ssh-ing to localhost. When I connect to localhost through ssh and when I try echo $JAVA_HOME, it prints the correct path to my Java home. I'm clueless at the moment. Okay, I figured out. It works. Yay! When deployment is going through SSH, it is a non-interctive shell, therefore /etc/profile is not executed. More details can be found here: http://askubuntu.com/questions/247738/why-is-etc-profile-not-invoked-for-non-login-shells When I added the following line before the line '# If not running interactively, don't do anything' in ~/.bashrc: ``` export JAVA_HOME=/path/to/java/home ``` Integration tests started working. Maybe it would be a good idea to mention this on the page where integration tests are described. - József Márton --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/#review92938 --- On July 27, 2015, 10:15 a.m., József Márton Jung wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/ --- (Updated July 27, 2015, 10:15 a.m.) Review request for samza. Repository: samza Description --- The following has been refactored: 1. Static inner classes from CoordinatorStreamMessage has been extracted 2. Common functionality from CheckpointManager, ChangelogMappingManager and LocalityManager has benn moved to a base class Diffs - checkstyle/import-control.xml 6654319 samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java 7445996 samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 55c258f samza-core/src/main/java/org/apache/samza/coordinator/stream/AbstractCoordinatorStreamManager.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamMessage.java e5ab4fb samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java b1078bd samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemProducer.java 92f8907 samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamWriter.java f769756 samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/CoordinatorStreamMessage.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/Delete.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetChangelogMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetCheckpoint.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetConfig.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetContainerHostMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/job/model/JobModel.java ad6387d samza-core/src/main/java/org/apache/samza/storage/ChangelogPartitionManager.java 7d3409c samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala f621611 samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 1c178a6 samza-core/src/test/java/org/apache/samza/coordinator/stream/MockCoordinatorStreamWrappedConsumer.java e454593 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamMessage.java ac26a01 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemConsumer.java c25f6a7 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemProducer.java
Re: Review Request 36545: SAMZA-682 Refactor Coordinator stream messages
On July 24, 2015, 6:01 p.m., Navina Ramesh wrote: Thanks for picking this up! It feels good to look at a refactored code. One suggestion: Please run all the intergration test (including the zopkio tests) before checking in this patch. I don't think we cleanly start and stop coordinator stream producers/consumers in all the managers. Please verify that nothing is broken due to this change. József Márton Jung wrote: I have difficulties running the zopkio tests. The error message is the following: 2015-07-27 11:36:44,608 zopkio.remote_host_helper [ERROR] Error: JAVA_HOME is not set and could not be found. JAVA_HOME is set on my machine (in /etc/profile, so it is available system-wide), echoing it outputs the path to Java installation. I don't knw what is wrong. Navina Ramesh wrote: I think the problem is with the remote host (host to which zopkio is trying to ssh) not having JAVA_HOME set correctly. Are you running the test on a remote machine? I'm trying to run the tests locally, so zopkio is ssh-ing to localhost. When I connect to localhost through ssh and when I try echo $JAVA_HOME, it prints the correct path to my Java home. I'm clueless at the moment. - József Márton --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/#review92938 --- On July 27, 2015, 10:15 a.m., József Márton Jung wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/ --- (Updated July 27, 2015, 10:15 a.m.) Review request for samza. Repository: samza Description --- The following has been refactored: 1. Static inner classes from CoordinatorStreamMessage has been extracted 2. Common functionality from CheckpointManager, ChangelogMappingManager and LocalityManager has benn moved to a base class Diffs - checkstyle/import-control.xml 6654319 samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java 7445996 samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 55c258f samza-core/src/main/java/org/apache/samza/coordinator/stream/AbstractCoordinatorStreamManager.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamMessage.java e5ab4fb samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java b1078bd samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemProducer.java 92f8907 samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamWriter.java f769756 samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/CoordinatorStreamMessage.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/Delete.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetChangelogMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetCheckpoint.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetConfig.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetContainerHostMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/job/model/JobModel.java ad6387d samza-core/src/main/java/org/apache/samza/storage/ChangelogPartitionManager.java 7d3409c samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala f621611 samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 1c178a6 samza-core/src/test/java/org/apache/samza/coordinator/stream/MockCoordinatorStreamWrappedConsumer.java e454593 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamMessage.java ac26a01 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemConsumer.java c25f6a7 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemProducer.java 1ef07d0 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamWriter.java c484660 samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala 84fdeaa samza-yarn/src/main/resources/scalate/WEB-INF/views/index.scaml 41303f7 Diff: https://reviews.apache.org/r/36545/diff/ Testing --- Tests has been updated. Thanks, József Márton Jung
Re: Review Request 36545: SAMZA-682 Refactor Coordinator stream messages
On July 24, 2015, 6:01 p.m., Navina Ramesh wrote: Thanks for picking this up! It feels good to look at a refactored code. One suggestion: Please run all the intergration test (including the zopkio tests) before checking in this patch. I don't think we cleanly start and stop coordinator stream producers/consumers in all the managers. Please verify that nothing is broken due to this change. József Márton Jung wrote: I have difficulties running the zopkio tests. The error message is the following: 2015-07-27 11:36:44,608 zopkio.remote_host_helper [ERROR] Error: JAVA_HOME is not set and could not be found. JAVA_HOME is set on my machine (in /etc/profile, so it is available system-wide), echoing it outputs the path to Java installation. I don't knw what is wrong. Navina Ramesh wrote: I think the problem is with the remote host (host to which zopkio is trying to ssh) not having JAVA_HOME set correctly. Are you running the test on a remote machine? József Márton Jung wrote: I'm trying to run the tests locally, so zopkio is ssh-ing to localhost. When I connect to localhost through ssh and when I try echo $JAVA_HOME, it prints the correct path to my Java home. I'm clueless at the moment. József Márton Jung wrote: Okay, I figured out. It works. Yay! When deployment is going through SSH, it is a non-interctive shell, therefore /etc/profile is not executed. More details can be found here: http://askubuntu.com/questions/247738/why-is-etc-profile-not-invoked-for-non-login-shells When I added the following line before the line '# If not running interactively, don't do anything' in ~/.bashrc: ``` export JAVA_HOME=/path/to/java/home ``` Integration tests started working. Maybe it would be a good idea to mention this on the page where integration tests are described. Duh.. that's true! It didn't occur to me. Glad you were able to figure it out. Yeah. We should probably mention this in the documentation. - Navina --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/#review92938 --- On July 27, 2015, 10:15 a.m., József Márton Jung wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/ --- (Updated July 27, 2015, 10:15 a.m.) Review request for samza. Repository: samza Description --- The following has been refactored: 1. Static inner classes from CoordinatorStreamMessage has been extracted 2. Common functionality from CheckpointManager, ChangelogMappingManager and LocalityManager has benn moved to a base class Diffs - checkstyle/import-control.xml 6654319 samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java 7445996 samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 55c258f samza-core/src/main/java/org/apache/samza/coordinator/stream/AbstractCoordinatorStreamManager.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamMessage.java e5ab4fb samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java b1078bd samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemProducer.java 92f8907 samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamWriter.java f769756 samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/CoordinatorStreamMessage.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/Delete.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetChangelogMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetCheckpoint.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetConfig.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetContainerHostMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/job/model/JobModel.java ad6387d samza-core/src/main/java/org/apache/samza/storage/ChangelogPartitionManager.java 7d3409c samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala f621611 samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 1c178a6 samza-core/src/test/java/org/apache/samza/coordinator/stream/MockCoordinatorStreamWrappedConsumer.java e454593 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamMessage.java ac26a01
Re: Review Request 36545: SAMZA-682 Refactor Coordinator stream messages
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/ --- (Updated July 27, 2015, 10:11 a.m.) Review request for samza. Changes --- Modifications based on Navina's comments. Repository: samza Description --- The following has been refactored: 1. Static inner classes from CoordinatorStreamMessage has been extracted 2. Common functionality from CheckpointManager, ChangelogMappingManager and LocalityManager has benn moved to a base class Diffs (updated) - checkstyle/import-control.xml 6654319 samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java 7445996 samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 55c258f samza-core/src/main/java/org/apache/samza/coordinator/stream/AbstractCoordinatorStreamManager.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamMessage.java e5ab4fb samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java b1078bd samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemProducer.java 92f8907 samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamWriter.java f769756 samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/CoordinatorStreamMessage.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/Delete.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetChangelogMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetCheckpoint.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetConfig.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetContainerHostMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/job/model/JobModel.java ad6387d samza-core/src/main/java/org/apache/samza/storage/ChangelogPartitionManager.java 7d3409c samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala f621611 samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 1c178a6 samza-core/src/test/java/org/apache/samza/coordinator/stream/MockCoordinatorStreamWrappedConsumer.java e454593 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamMessage.java ac26a01 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemConsumer.java c25f6a7 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemProducer.java 1ef07d0 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamWriter.java c484660 samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala 84fdeaa samza-yarn/src/main/resources/scalate/WEB-INF/views/index.scaml 41303f7 Diff: https://reviews.apache.org/r/36545/diff/ Testing --- Tests has been updated. Thanks, József Márton Jung
Re: Review Request 36545: SAMZA-682 Refactor Coordinator stream messages
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/#review92938 --- Thanks for picking this up! It feels good to look at a refactored code. One suggestion: Please run all the intergration test (including the zopkio tests) before checking in this patch. I don't think we cleanly start and stop coordinator stream producers/consumers in all the managers. Please verify that nothing is broken due to this change. samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 627) https://reviews.apache.org/r/36545/#comment147198 LocalityManager maintains container to host-level mapping. Not a task to host mapping. Please change this back to containerId. - Navina Ramesh On July 24, 2015, 12:27 p.m., József Márton Jung wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/ --- (Updated July 24, 2015, 12:27 p.m.) Review request for samza. Repository: samza Description --- The following has been refactored: 1. Static inner classes from CoordinatorStreamMessage has been extracted 2. Common functionality from CheckpointManager, ChangelogMappingManager and LocalityManager has benn moved to a base class Diffs - checkstyle/import-control.xml 6654319 samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java 7445996 samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 55c258f samza-core/src/main/java/org/apache/samza/coordinator/stream/AbstractCoordinatorStreamManager.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamMessage.java e5ab4fb samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java b1078bd samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemProducer.java 92f8907 samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamWriter.java f769756 samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/CoordinatorStreamMessage.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/Delete.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetChangelogMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetCheckpoint.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetConfig.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetContainerHostMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/job/model/JobModel.java ad6387d samza-core/src/main/java/org/apache/samza/storage/ChangelogPartitionManager.java 7d3409c samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 27b2517 samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala f621611 samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 1c178a6 samza-core/src/test/java/org/apache/samza/coordinator/stream/MockCoordinatorStreamWrappedConsumer.java e454593 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamMessage.java ac26a01 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemConsumer.java c25f6a7 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemProducer.java 1ef07d0 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamWriter.java c484660 samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala 84fdeaa samza-yarn/src/main/resources/scalate/WEB-INF/views/index.scaml 41303f7 Diff: https://reviews.apache.org/r/36545/diff/ Testing --- Tests has been updated. Thanks, József Márton Jung
Re: Review Request 36545: SAMZA-682 Refactor Coordinator stream messages
On July 23, 2015, 10:35 p.m., Yan Fang wrote: samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java, line 46 https://reviews.apache.org/r/36545/diff/1/?file=1013337#file1013337line46 To be consistent, lets go with TaskName, not the String. Corrected. On July 23, 2015, 10:35 p.m., Yan Fang wrote: samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java, line 74 https://reviews.apache.org/r/36545/diff/1/?file=1013337#file1013337line74 Any reason that you do not want to use the TaskName class? TaskName seems fine here. Since the TaskName class consists of only one field called taskName, I tought that using only a string is sufficient. Chaned back to TaskName. On July 23, 2015, 10:35 p.m., Yan Fang wrote: samza-core/src/main/java/org/apache/samza/container/LocalityManager.java, line 64 https://reviews.apache.org/r/36545/diff/1/?file=1013338#file1013338line64 sourceSuffix is more descriptive. Corrected. On July 23, 2015, 10:35 p.m., Yan Fang wrote: samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java, line 20 https://reviews.apache.org/r/36545/diff/1/?file=1013349#file1013349line20 I think it makes sense that this class stays in its original package: samza-core/src/main/java/org/apache/samza/coordinator/stream . Because its only about the coordinatorStream, not the overall manager of the samza. Corrected, moved the class to samza-core/src/main/java/org/apache/samza/coordinator/stream package. On July 23, 2015, 10:35 p.m., Yan Fang wrote: samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java, line 29 https://reviews.apache.org/r/36545/diff/1/?file=1013349#file1013349line29 a little more in the doc. This class is not really manages the coordinator stream, it is an abstract class that other stream managers want to extend. Also, renaming it to AbstractCoordinatorStreamManager maybe helpful too. Renamed. Also Javadoc updated on the class. On July 23, 2015, 10:35 p.m., Yan Fang wrote: samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java, line 65 https://reviews.apache.org/r/36545/diff/1/?file=1013349#file1013349line65 typo, sends Corrected. On July 23, 2015, 10:35 p.m., Yan Fang wrote: samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java, line 96 https://reviews.apache.org/r/36545/diff/1/?file=1013349#file1013349line96 no + Removed. On July 23, 2015, 10:35 p.m., Yan Fang wrote: samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java, line 112 https://reviews.apache.org/r/36545/diff/1/?file=1013349#file1013349line112 I think, taskName maybe more general. In case we have more information in the TaskName, or other rules of registering. Just personal idea. I agree. Renamed to taskName. On July 23, 2015, 10:35 p.m., Yan Fang wrote: samza-core/src/main/java/org/apache/samza/storage/ChangelogPartitionManager.java, line 74 https://reviews.apache.org/r/36545/diff/1/?file=1013350#file1013350line74 going with the taskName is fine. Corrected. On July 23, 2015, 10:35 p.m., Yan Fang wrote: samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala, line 151 https://reviews.apache.org/r/36545/diff/1/?file=1013351#file1013351line151 if we use TaskName in the regitser method, do not need to change this one. Corrected. On July 23, 2015, 10:35 p.m., Yan Fang wrote: samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala, line 245 https://reviews.apache.org/r/36545/diff/1/?file=1013352#file1013352line245 same Corrected. On July 23, 2015, 10:35 p.m., Yan Fang wrote: samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line 185 https://reviews.apache.org/r/36545/diff/1/?file=1013353#file1013353line185 same Corrected. - József Márton --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/#review92825 --- On July 16, 2015, 1:33 p.m., József Márton Jung wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/ --- (Updated July 16, 2015, 1:33 p.m.) Review request for samza. Repository: samza Description --- The following has been refactored: 1. Static inner classes from CoordinatorStreamMessage has been extracted 2. Common functionality from CheckpointManager, ChangelogMappingManager and LocalityManager has benn moved to a base class Diffs - checkstyle/import-control.xml eef3370
Re: Review Request 36545: SAMZA-682 Refactor Coordinator stream messages
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/ --- (Updated July 24, 2015, 12:27 p.m.) Review request for samza. Changes --- Corrections based on code review. Repository: samza Description --- The following has been refactored: 1. Static inner classes from CoordinatorStreamMessage has been extracted 2. Common functionality from CheckpointManager, ChangelogMappingManager and LocalityManager has benn moved to a base class Diffs (updated) - checkstyle/import-control.xml 6654319 samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java 7445996 samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 55c258f samza-core/src/main/java/org/apache/samza/coordinator/stream/AbstractCoordinatorStreamManager.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamMessage.java e5ab4fb samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java b1078bd samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemProducer.java 92f8907 samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamWriter.java f769756 samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/CoordinatorStreamMessage.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/Delete.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetChangelogMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetCheckpoint.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetConfig.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetContainerHostMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/job/model/JobModel.java ad6387d samza-core/src/main/java/org/apache/samza/storage/ChangelogPartitionManager.java 7d3409c samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 27b2517 samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala f621611 samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 1c178a6 samza-core/src/test/java/org/apache/samza/coordinator/stream/MockCoordinatorStreamWrappedConsumer.java e454593 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamMessage.java ac26a01 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemConsumer.java c25f6a7 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemProducer.java 1ef07d0 samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamWriter.java c484660 samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala 84fdeaa samza-yarn/src/main/resources/scalate/WEB-INF/views/index.scaml 41303f7 Diff: https://reviews.apache.org/r/36545/diff/ Testing --- Tests has been updated. Thanks, József Márton Jung
Re: Review Request 36545: SAMZA-682 Refactor Coordinator stream messages
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/#review92825 --- samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java (line 43) https://reviews.apache.org/r/36545/#comment147063 To be consistent, lets go with TaskName, not the String. samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java (line 57) https://reviews.apache.org/r/36545/#comment147064 Any reason that you do not want to use the TaskName class? TaskName seems fine here. samza-core/src/main/java/org/apache/samza/container/LocalityManager.java (line 50) https://reviews.apache.org/r/36545/#comment147065 sourceSuffix is more descriptive. samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java (line 20) https://reviews.apache.org/r/36545/#comment147066 I think it makes sense that this class stays in its original package: samza-core/src/main/java/org/apache/samza/coordinator/stream . Because its only about the coordinatorStream, not the overall manager of the samza. samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java (line 29) https://reviews.apache.org/r/36545/#comment147071 a little more in the doc. This class is not really manages the coordinator stream, it is an abstract class that other stream managers want to extend. Also, renaming it to AbstractCoordinatorStreamManager maybe helpful too. samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java (line 65) https://reviews.apache.org/r/36545/#comment147072 typo, sends samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java (line 96) https://reviews.apache.org/r/36545/#comment147073 no + samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java (line 112) https://reviews.apache.org/r/36545/#comment147074 I think, taskName maybe more general. In case we have more information in the TaskName, or other rules of registering. Just personal idea. samza-core/src/main/java/org/apache/samza/storage/ChangelogPartitionManager.java (line 52) https://reviews.apache.org/r/36545/#comment147067 going with the taskName is fine. samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala (line 151) https://reviews.apache.org/r/36545/#comment147068 if we use TaskName in the regitser method, do not need to change this one. samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala (line 245) https://reviews.apache.org/r/36545/#comment147069 same samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 185) https://reviews.apache.org/r/36545/#comment147070 same - Yan Fang On July 16, 2015, 1:33 p.m., József Márton Jung wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/ --- (Updated July 16, 2015, 1:33 p.m.) Review request for samza. Repository: samza Description --- The following has been refactored: 1. Static inner classes from CoordinatorStreamMessage has been extracted 2. Common functionality from CheckpointManager, ChangelogMappingManager and LocalityManager has benn moved to a base class Diffs - checkstyle/import-control.xml eef3370 samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java 7445996 samza-core/src/main/java/org/apache/samza/container/LocalityManager.java 55c258f samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamMessage.java 6bd1bd3 samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java b1078bd samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemProducer.java 92f8907 samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/CoordinatorStreamMessage.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/Delete.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetChangelogMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetCheckpoint.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetConfig.java PRE-CREATION samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetContainerHostMapping.java PRE-CREATION samza-core/src/main/java/org/apache/samza/job/model/JobModel.java ad6387d samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java PRE-CREATION
