You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2020/07/17 09:12:05 UTC
[GitHub] [ignite] vldpyatkov opened a new pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
vldpyatkov opened a new pull request #8048:
URL: https://github.com/apache/ignite/pull/8048
… more rows than required
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
----------------------------------------------------------------
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
[GitHub] [ignite] vldpyatkov commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
vldpyatkov commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r475499005
##########
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:
Done.
----------------------------------------------------------------
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
[GitHub] [ignite] ascherbakoff commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r475524877
##########
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:
"By that we can see only previous checkpoint (not deeper)." - We will go deeper as needed until find a cp covering cntr - margin or go beyond the history. As it works right now.
I've tested my code and it fixes your reproducer.
"Not need to look at a checkpoint if newer have not any update of particular cache (even if we have not margin enough)." - Can you add the proposed minPtr search algorithm description in the ticket description and the javadoc to the method searchEarliestWalPointer ? Current solution seems to stop searching if previous segment does not hold data for `cntr - margin`, which looks ok.
----------------------------------------------------------------
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
[GitHub] [ignite] ascherbakoff commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r480180017
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/preloader/GridDhtPartitionsExchangeFuture.java
##########
@@ -3540,7 +3540,9 @@ private void resetOwnersByCounter(GridDhtPartitionTopology top,
long maxCntr = maxCntrObj != null ? maxCntrObj.cnt : 0;
- NavigableSet<Long> nonMaxCntrs = e.getValue().headSet(maxCntr, false);
+ NavigableSet<Long> nonMaxCntrs = e.getValue().headSet(maxCntr, false)
+ //Empty partition cannot be rebalanced by history effectively.
Review comment:
Space required.
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/wal/WalRecoveryTxLogicalRecordsTest.java
##########
@@ -370,6 +371,9 @@ public void testHistoricalRebalanceIterator() throws Exception {
map = new IgniteDhtDemandedPartitionsMap();
map.addHistorical(0, i, entries, PARTS);
+ GridTestUtils.setFieldValue(grp.shared().database(), "reservedForPreloading",
+ new FileWALPointer(0,0,0));
Review comment:
Bad formatting.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/checkpoint/CheckpointHistory.java
##########
@@ -496,9 +522,81 @@ private long absFileIdx(CheckpointEntry pointer) {
+ entry.getKey() + ", partCntrSince=" + entry.getValue() + "]");
}
+ while (!F.isEmpty(historyPointerCandidate)) {
+ FileWALPointer ptr = historyPointerCandidate.poll()
+ .choose(null, margin, partsCounter);
+
+ if (minPtr == null || ptr.compareTo(minPtr) < 0)
+ minPtr = ptr;
+ }
+
Review comment:
Copy paste: lines 475-481 are the same.
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/wal/WalRecoveryTxLogicalRecordsTest.java
##########
@@ -406,6 +416,9 @@ public void testHistoricalRebalanceIterator() throws Exception {
assertFalse(it.hasNext());
}
+ finally {
+ GridTestUtils.setFieldValue(grp.shared().database(), "reservedForPreloading",null);
Review comment:
Bad formatting.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/GridCacheOffheapManager.java
##########
@@ -122,6 +122,16 @@
* Used when persistence enabled.
*/
public class GridCacheOffheapManager extends IgniteCacheOffheapManagerImpl implements DbCheckpointListener {
+ /**
+ * 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 final long walAtomicCacheMargin = IgniteSystemProperties.getLong(
Review comment:
Why this is public ?
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/wal/WalRecoveryTxLogicalRecordsTest.java
##########
@@ -451,10 +467,16 @@ public void testHistoricalRebalanceIterator() throws Exception {
assertFalse(it.hasNext());
}
+ finally {
+ GridTestUtils.setFieldValue(grp.shared().database(), "reservedForPreloading",null);
+ }
map = new IgniteDhtDemandedPartitionsMap();
map.addHistorical(1, i, entries, PARTS);
+ GridTestUtils.setFieldValue(grp.shared().database(), "reservedForPreloading",
+ new FileWALPointer(0,0,0));
Review comment:
Bad formatting.
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/wal/WalRecoveryTxLogicalRecordsTest.java
##########
@@ -386,10 +390,16 @@ public void testHistoricalRebalanceIterator() throws Exception {
assertFalse(it.hasNext());
}
+ finally {
+ GridTestUtils.setFieldValue(grp.shared().database(), "reservedForPreloading",null);
+ }
map = new IgniteDhtDemandedPartitionsMap();
map.addHistorical(1, i, entries, PARTS);
+ GridTestUtils.setFieldValue(grp.shared().database(), "reservedForPreloading",
+ new FileWALPointer(0,0,0));
Review comment:
Bad formatting.
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/wal/WalRecoveryTxLogicalRecordsTest.java
##########
@@ -451,10 +467,16 @@ public void testHistoricalRebalanceIterator() throws Exception {
assertFalse(it.hasNext());
}
+ finally {
+ GridTestUtils.setFieldValue(grp.shared().database(), "reservedForPreloading",null);
Review comment:
Bad formatting.
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/wal/WalRecoveryTxLogicalRecordsTest.java
##########
@@ -961,12 +986,18 @@ private int prepareTx(Ignite ignite, List<Integer> keys) throws IgniteCheckedExc
List<CacheDataRow> rows = new ArrayList<>();
+ GridTestUtils.setFieldValue(grp.shared().database(), "reservedForPreloading",
+ new FileWALPointer(0,0,0));
Review comment:
Bad formatting.
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/wal/WalRecoveryTxLogicalRecordsTest.java
##########
@@ -425,6 +438,9 @@ public void testHistoricalRebalanceIterator() throws Exception {
map = new IgniteDhtDemandedPartitionsMap();
map.addHistorical(0, i, entries, PARTS);
+ GridTestUtils.setFieldValue(grp.shared().database(), "reservedForPreloading",
+ new FileWALPointer(0,0,0));
Review comment:
Bad formatting.
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/wal/WalRecoveryTxLogicalRecordsTest.java
##########
@@ -961,12 +986,18 @@ private int prepareTx(Ignite ignite, List<Integer> keys) throws IgniteCheckedExc
List<CacheDataRow> rows = new ArrayList<>();
+ GridTestUtils.setFieldValue(grp.shared().database(), "reservedForPreloading",
+ new FileWALPointer(0,0,0));
+
try (IgniteRebalanceIterator it = offh.rebalanceIterator(map, topVer)) {
assertNotNull(it);
while (it.hasNextX())
rows.add(it.next());
}
+ finally {
+ GridTestUtils.setFieldValue(grp.shared().database(), "reservedForPreloading",null);
Review comment:
Bad formatting.
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/wal/WalRecoveryTxLogicalRecordsTest.java
##########
@@ -386,10 +390,16 @@ public void testHistoricalRebalanceIterator() throws Exception {
assertFalse(it.hasNext());
}
+ finally {
+ GridTestUtils.setFieldValue(grp.shared().database(), "reservedForPreloading",null);
Review comment:
Bad formatting.
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/wal/WalRecoveryTxLogicalRecordsTest.java
##########
@@ -471,6 +493,9 @@ public void testHistoricalRebalanceIterator() throws Exception {
assertFalse(it.hasNext());
}
+ finally {
+ GridTestUtils.setFieldValue(grp.shared().database(), "reservedForPreloading",null);
Review comment:
Bad formatting.
##########
File path: modules/core/src/test/java/org/apache/ignite/cache/ResetLostPartitionTest.java
##########
@@ -68,26 +68,26 @@
/** {@inheritDoc} */
@Override protected void afterTest() throws Exception {
- super.afterTest();
-
stopAllGrids();
cleanPersistenceDir();
+
+ super.afterTest();
Review comment:
What's the reason for the change ? It doesn't seem correct to me.
##########
File path: modules/core/src/test/java/org/apache/ignite/cache/ResetLostPartitionTest.java
##########
@@ -262,8 +262,9 @@ private int averageSizeAroundAllNodes() {
int totalSize = 0;
for (Ignite ignite : IgnitionEx.allGrids()) {
- for (String cacheName : CACHE_NAMES)
+ for (String cacheName : CACHE_NAMES) {
Review comment:
Brackets should be removed.
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/distributed/CachePartitionLostAfterSupplierHasLeftTest.java
##########
@@ -108,11 +108,11 @@
* {@inheritDoc}
*/
@Override protected void afterTest() throws Exception {
- super.afterTest();
-
stopAllGrids();
cleanPersistenceDir();
+
+ super.afterTest();
Review comment:
What's the reason for the change ? It doesn't seem correct to me.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/checkpoint/CheckpointHistory.java
##########
@@ -473,20 +492,27 @@ private long absFileIdx(CheckpointEntry pointer) {
if (foundCntr != null && foundCntr <= entry.getValue()) {
iter.remove();
- FileWALPointer ptr = (FileWALPointer)cpEntry.checkpointMark();
-
if (ptr == null) {
throw new IgniteCheckedException("Could not find start pointer for partition [part="
+ entry.getKey() + ", partCntrSince=" + entry.getValue() + "]");
}
+ if (foundCntr + margin > entry.getValue()) {
+ historyPointerCandidate.add(new WalPointerCandidate(grpId, entry.getKey(), entry.getValue(), ptr,
+ foundCntr));
+
+ continue;
+ }
+
+ partsCounter.put(entry.getKey(), entry.getValue() - margin);
+
if (minPtr == null || ptr.compareTo(minPtr) < 0)
minPtr = ptr;
}
}
- if (F.isEmpty(modifiedPartsCounter))
- return minPtr;
+ if ((F.isEmpty(modifiedPartsCounter) && F.isEmpty(historyPointerCandidate)) || ptr.compareTo(latestReservedPointer) <= 0)
Review comment:
Why this condifition is required: ptr.compareTo(latestReservedPointer) <= 0 ?
For me it will be necessary to check for equality. because latest reserved pointer is a checkpoint mark:
ptr.compareTo(latestReservedPointer) == 0
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/GridCacheOffheapManager.java
##########
@@ -1013,6 +1023,16 @@ private Metas getOrAllocateCacheMetas() throws IgniteCheckedException {
if (grp.mvccEnabled()) // TODO IGNITE-7384
return super.historicalIterator(partCntrs, missing);
+ GridCacheDatabaseSharedManager database = (GridCacheDatabaseSharedManager)grp.shared().database();
+
+ FileWALPointer latestReservedPointer = (FileWALPointer)database.latestWalPointerReservedForPreloading();
+
+ if (latestReservedPointer == null) {
+ log.warning("Historical iterator wasn't created, because WAL isn't reserved.");
+
+ return null;
Review comment:
Should't we throw here IgniteHistoricalIteratorException to fallback to full rebalancing ?
----------------------------------------------------------------
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
[GitHub] [ignite] ascherbakoff commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r475527253
##########
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:
No it doesn't for 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:
Still not clear. Your english is difficult to understand.
"By that we can see only previous checkpoint (not deeper)." - We will go deeper as needed until find a cp covering cntr - margin or go beyond the history. As it works right now.
Can you provide a test scenario where my logic is broken ? I've tested my code and it fixes your reproducer.
"Not need to look at a checkpoint if newer have not any update of particular cache (even if we have not margin enough)." - I don't understand.
----------------------------------------------------------------
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
[GitHub] [ignite] ascherbakoff commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r475524877
##########
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:
"By that we can see only previous checkpoint (not deeper)." - We will go deeper as needed until find a cp covering cntr - margin or go beyond the history. As it works right now.
I've tested my code and it fixes your reproducer.
"Not need to look at a checkpoint if newer have not any update of particular cache (even if we have not margin enough)." - Can you add the proposed algorithm description in the ticket and the javadoc to the method searchEarliestWalPointer ? I need to think about it.
----------------------------------------------------------------
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
[GitHub] [ignite] vldpyatkov commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
vldpyatkov commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r475483128
##########
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 think it looks just right system property. Because we have no information what this value can by.
But in a test where we suspect influence of reordering we can increase this parameter.
----------------------------------------------------------------
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
[GitHub] [ignite] ascherbakoff commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r475524877
##########
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:
"By that we can see only previous checkpoint (not deeper)." - We will go deeper as needed until find a cp covering cntr - margin or go beyond the history. As it works right now.
I've tested my code and it fixes your reproducer.
"Not need to look at a checkpoint if newer have not any update of particular cache (even if we have not margin enough)." - Can you add the proposed algorythm description in the ticket and javadoc to the method searchEarliestWalPointer ? I have to think about it.
----------------------------------------------------------------
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
[GitHub] [ignite] vldpyatkov commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
vldpyatkov commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r482887277
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/GridCacheOffheapManager.java
##########
@@ -1013,6 +1023,16 @@ private Metas getOrAllocateCacheMetas() throws IgniteCheckedException {
if (grp.mvccEnabled()) // TODO IGNITE-7384
return super.historicalIterator(partCntrs, missing);
+ GridCacheDatabaseSharedManager database = (GridCacheDatabaseSharedManager)grp.shared().database();
+
+ FileWALPointer latestReservedPointer = (FileWALPointer)database.latestWalPointerReservedForPreloading();
+
+ if (latestReservedPointer == null) {
+ log.warning("Historical iterator wasn't created, because WAL isn't reserved.");
+
+ return null;
Review comment:
Good point, I will throw the exception here.
----------------------------------------------------------------
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
[GitHub] [ignite] vldpyatkov commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
vldpyatkov commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r482875119
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/db/wal/WalRecoveryTxLogicalRecordsTest.java
##########
@@ -370,6 +371,9 @@ public void testHistoricalRebalanceIterator() throws Exception {
map = new IgniteDhtDemandedPartitionsMap();
map.addHistorical(0, i, entries, PARTS);
+ GridTestUtils.setFieldValue(grp.shared().database(), "reservedForPreloading",
+ new FileWALPointer(0,0,0));
Review comment:
Resolved all format issues here.
----------------------------------------------------------------
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
[GitHub] [ignite] vldpyatkov commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
vldpyatkov commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r482893676
##########
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:
Done.
----------------------------------------------------------------
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
[GitHub] [ignite] vldpyatkov commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
vldpyatkov commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r482893278
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/checkpoint/CheckpointHistory.java
##########
@@ -473,20 +492,27 @@ private long absFileIdx(CheckpointEntry pointer) {
if (foundCntr != null && foundCntr <= entry.getValue()) {
iter.remove();
- FileWALPointer ptr = (FileWALPointer)cpEntry.checkpointMark();
-
if (ptr == null) {
throw new IgniteCheckedException("Could not find start pointer for partition [part="
+ entry.getKey() + ", partCntrSince=" + entry.getValue() + "]");
}
+ if (foundCntr + margin > entry.getValue()) {
+ historyPointerCandidate.add(new WalPointerCandidate(grpId, entry.getKey(), entry.getValue(), ptr,
+ foundCntr));
+
+ continue;
+ }
+
+ partsCounter.put(entry.getKey(), entry.getValue() - margin);
+
if (minPtr == null || ptr.compareTo(minPtr) < 0)
minPtr = ptr;
}
}
- if (F.isEmpty(modifiedPartsCounter))
- return minPtr;
+ if ((F.isEmpty(modifiedPartsCounter) && F.isEmpty(historyPointerCandidate)) || ptr.compareTo(latestReservedPointer) <= 0)
Review comment:
You right I am changing this, the check happens after a WAL pointer might be assigned (it will be corrected only when the pointer is ">=" of the bound).
----------------------------------------------------------------
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
[GitHub] [ignite] vldpyatkov commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
vldpyatkov commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r475487550
##########
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:
This logic is real more complicated than you advise. By that we can see only previous checkpoint (not deeper).
Not need to look at a checkpoint if newer have not any update of particular cache (even if we have not margin enough).
----------------------------------------------------------------
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
[GitHub] [ignite] ascherbakoff commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r475524877
##########
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:
"By that we can see only previous checkpoint (not deeper)." - We will go deeper as needed until find a cp covering cntr - margin or go beyond the history. As it works right now.
I've tested my code and it fixes your reproducer.
"Not need to look at a checkpoint if newer have not any update of particular cache (even if we have not margin enough)." - Can you add the proposed minPtr search algorithm description in the ticket description and the javadoc to the method searchEarliestWalPointer ? I need to decide if it's ok. Current solution seems to stop searching if previous segment does not hold data for `cntr - margin`.
----------------------------------------------------------------
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
[GitHub] [ignite] ascherbakoff commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r475524877
##########
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:
"By that we can see only previous checkpoint (not deeper)." - We will go deeper as needed until find a cp covering cntr - margin or go beyond the history. As it works right now.
I've tested my code and it fixes your reproducer.
"Not need to look at a checkpoint if newer have not any update of particular cache (even if we have not margin enough)." - Can you add the proposed minPtr search algorithm description in the ticket and the javadoc to the method searchEarliestWalPointer ? I need to decide if it's ok. Current solution seems to stop searching if previous segment does not hold data for `cntr - margin`.
----------------------------------------------------------------
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
[GitHub] [ignite] ascherbakoff commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r475524877
##########
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:
Still not clear. Your english is difficult to understand.
"By that we can see only previous checkpoint (not deeper)." - We will go deeper as needed until find a cp covering cntr - margin or go beyond the history. As it works right now.
Can you provide a test scenario where my logic is broken ? I've tested my code and it fixes your reproducer.
"Not need to look at a checkpoint if newer have not any update of particular cache (even if we have not margin enough)." - Can you add the proposed algorythm description in the ticket and javadoc to the method searchEarliestWalPointer ?
----------------------------------------------------------------
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
[GitHub] [ignite] dspavlov commented on pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
dspavlov commented on pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#issuecomment-894334647
This PR is associated with a resolved ticket. Closing the PR, please reopen it if you still need it.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] vldpyatkov commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
vldpyatkov commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r482879479
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/distributed/CachePartitionLostAfterSupplierHasLeftTest.java
##########
@@ -108,11 +108,11 @@
* {@inheritDoc}
*/
@Override protected void afterTest() throws Exception {
- super.afterTest();
-
stopAllGrids();
cleanPersistenceDir();
+
+ super.afterTest();
Review comment:
Ok.
----------------------------------------------------------------
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
[GitHub] [ignite] ascherbakoff commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r475524877
##########
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:
"By that we can see only previous checkpoint (not deeper)." - We will go deeper as needed until find a cp covering cntr - margin or go beyond the history. As it works right now.
I've tested my code and it fixes your reproducer.
"Not need to look at a checkpoint if newer have not any update of particular cache (even if we have not margin enough)." - Can you add the proposed algorithm description in the ticket and the javadoc to the method searchEarliestWalPointer ? I have to think about it.
----------------------------------------------------------------
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
[GitHub] [ignite] vldpyatkov commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
vldpyatkov commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r482873669
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/GridCacheOffheapManager.java
##########
@@ -122,6 +122,16 @@
* Used when persistence enabled.
*/
public class GridCacheOffheapManager extends IgniteCacheOffheapManagerImpl implements DbCheckpointListener {
+ /**
+ * 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 final long walAtomicCacheMargin = IgniteSystemProperties.getLong(
Review comment:
Made it private.
----------------------------------------------------------------
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
[GitHub] [ignite] ascherbakoff commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by 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
[GitHub] [ignite] vldpyatkov commented on a change in pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
vldpyatkov commented on a change in pull request #8048:
URL: https://github.com/apache/ignite/pull/8048#discussion_r482873062
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/preloader/GridDhtPartitionsExchangeFuture.java
##########
@@ -3540,7 +3540,9 @@ private void resetOwnersByCounter(GridDhtPartitionTopology top,
long maxCntr = maxCntrObj != null ? maxCntrObj.cnt : 0;
- NavigableSet<Long> nonMaxCntrs = e.getValue().headSet(maxCntr, false);
+ NavigableSet<Long> nonMaxCntrs = e.getValue().headSet(maxCntr, false)
+ //Empty partition cannot be rebalanced by history effectively.
Review comment:
Done.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/checkpoint/CheckpointHistory.java
##########
@@ -496,9 +522,81 @@ private long absFileIdx(CheckpointEntry pointer) {
+ entry.getKey() + ", partCntrSince=" + entry.getValue() + "]");
}
+ while (!F.isEmpty(historyPointerCandidate)) {
+ FileWALPointer ptr = historyPointerCandidate.poll()
+ .choose(null, margin, partsCounter);
+
+ if (minPtr == null || ptr.compareTo(minPtr) < 0)
+ minPtr = ptr;
+ }
+
Review comment:
Done.
----------------------------------------------------------------
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
[GitHub] [ignite] dspavlov closed pull request #8048: IGNITE-13265 Historical iterator for atomic group should transfer few…
Posted by GitBox <gi...@apache.org>.
dspavlov closed pull request #8048:
URL: https://github.com/apache/ignite/pull/8048
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org