[GitHub] [ignite] timoninmaxim commented on a diff in pull request #10396: Ignite-17793 : Historical rebalance must use HWM instead of LWM, v2: store pending counter instead

[GitBox]

timoninmaxim commented on code in PR #10396:
URL: https://github.com/apache/ignite/pull/10396#discussion_r1039330396


##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/checkpoint/CheckpointWorkflow.java:
##########
@@ -396,10 +396,13 @@ private void fillCacheGroupState(CheckpointRecord cpRec) 
throws IgniteCheckedExc
                     if (partState == LOST)
                         partState = OWNING;
 
+                    assert part.highestAppliedCounter() >= 
part.updateCounter() :

Review Comment:
   I'm not sure we need this assert:
   1. PartitionCounter implementation should guarantee that by default, 
otherwise it's a bug. 
   2. `part.highestAppliedCounter()` is a synchronized method, and you invoke 
it twice here.
   3. I have concerns for this expression, is there guarantees that there are 
no concurrent updates of partition counter between invokation 
`highestAppliedCounter` and invokation `updateCounter`?
   
   WDYT?



-- 
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.

To unsubscribe, e-mail: notifications-unsubscr...@ignite.apache.org

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