[GitHub] [samza] mynameborat commented on a change in pull request #1484: SAMZA-2633: Rolling upgrades cause downtime to upgraded processors for the entire deployment window

[GitBox <gi...@apache.org>]

mynameborat commented on a change in pull request #1484:
URL: https://github.com/apache/samza/pull/1484#discussion_r606422558



##########
File path: samza-core/src/main/java/org/apache/samza/zk/ZkJobCoordinator.java
##########
@@ -274,6 +283,20 @@ void doOnProcessorChange() {
     LOG.info("Generating new JobModel with processors: {}.", currentProcessorIds);
     JobModel newJobModel = generateNewJobModel(processorNodes);
 
+    /*
+     * Leader skips the rebalance even if there are changes in the quorum as long as the work assignment remains the same
+     * across all the processors. The optimization is useful in the following scenarios
+     *   1. The processor in the quorum restarts within the debounce window. Originally, this would trigger rebalance
+     *      across the processors stopping and starting their work assignment which is detrimental to availability of
+     *      the system. e.g. common scenario during rolling upgrades
+     *   2. Processors in the quorum which don't have work assignment and their failures/restarts don't impact the
+     *      quorum.
+     */
+    if (newJobModel.equals(activeJobModel)) {

Review comment:
       Good point. I suppose the reason it works during my testing is `generateNewJobModel` doesn't store the configuration with the job model generated and hence the check happens to be the same as comparing the container model for all the processors. 
   
   I will make it explicit to avoid this implicit assumption.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org