[GitHub] [ignite] ascherbakoff commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…

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

ascherbakoff commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r475434651



##########
File path: modules/core/src/main/java/org/apache/ignite/IgniteSystemProperties.java
##########
@@ -753,6 +753,16 @@
      */
     public static final String IGNITE_PREFER_WAL_REBALANCE = "IGNITE_PREFER_WAL_REBALANCE";
 
+    /**
+     * Margin for WAL iterator, that used for historical rebalance on atomic cache.
+     * It is intended for prevent  partition divergence due to reordering in WAL.
+     * <p>
+     * Default is {@code 5}. Iterator starts from 5 updates earlier than expected.
+     *
+     */
+    public static final String WAL_MARGIN_FOR_ATOMIC_CACHE_HISTORICAL_REBALANCE =

Review comment:
       I'm against making this a public property because it's impossible to set "correct" value for it. Moreover, it referes to some Ignite internals which are cetainly a subject to change in the future releases. Make this an internal constant. 5 looks a reasonable heuristic to me. 

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/checkpoint/CheckpointHistory.java
##########
@@ -451,17 +452,31 @@ private long absFileIdx(CheckpointEntry pointer) {
      * @param partsCounter Partition mapped to update counter.
      * @return Earliest WAL pointer for group specified.
      */
-    @Nullable public WALPointer searchEarliestWalPointer(int grpId, Map<Integer, Long> partsCounter) throws IgniteCheckedException {
+    @Nullable public WALPointer searchEarliestWalPointer(

Review comment:
       The introduced changes in searchEarliestWalPointer look overcomplicated and unnecessary.
   I tried a much simplier solution and it seems working:
   
   ```
   long margin = grp.hasAtomicCaches() ? walAtomicCacheMargin : 0L;
           
   Map<Integer, Long> partsCounters0 = F.viewReadOnly(partsCounters, new IgniteBiClosure<Integer, Long, Long>() {
               @Override public Long apply(Integer p, Long cntr) {
                   return Math.max(0, cntr - margin);
               }
           }, F.alwaysTrue());
   
   GridCacheDatabaseSharedManager database = (GridCacheDatabaseSharedManager)grp.shared().database();
   
   FileWALPointer minPtr = (FileWALPointer)database.checkpointHistory().searchEarliestWalPointer(grp.groupId(),
               partsCounters0);
   ```
   
   There is a catch here - it's possible the `cntr - margin` is located beyond the reserved history. In such a case we should immediately return a pointer to a start of a history. This is the only change required in searchEarliestWalPointer.
   

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/preloader/CachePartitionPartialCountersMap.java
##########
@@ -177,6 +177,17 @@ public long initialUpdateCounterAt(int idx) {
         return initialUpdCntrs[idx];
     }
 
+    /**
+     * Update initial counter by given index.
+     * It is used when iterated by WAL with a margin.
+     *
+     * @param idx Index.
+     * @param cntr Counter.
+     */
+    public void setInitialUpdateCounterAt(int idx, long cntr) {

Review comment:
       Broken codestyle. The method should be named initialUpdateCounterAt(idx, cntr)




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