timoninmaxim commented on a change in pull request #9661:
URL: https://github.com/apache/ignite/pull/9661#discussion_r773790497
##########
File path:
modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerConsistencyTest.java
##########
@@ -187,36 +215,48 @@ public void testRepairNonExistentCache() throws Exception
{
/**
*
*/
- private void readRepairTx(AtomicInteger brokenParts, String cacheName) {
+ private void readRepair(AtomicInteger brokenParts, String cacheName,
Integer fixesPerEntry) {
for (int i = 0; i < PARTITIONS; i++) {
- assertEquals(EXIT_CODE_OK, execute("--consistency", "repair",
cacheName, String.valueOf(i)));
+ assertEquals(EXIT_CODE_OK, execute("--consistency", "repair",
cacheName, String.valueOf(i), strategy.toString()));
assertContains(log, testOut.toString(),
CONSISTENCY_VIOLATIONS_FOUND);
- assertContains(log, testOut.toString(), "[found=1, fixed=1");
+ assertContains(log, testOut.toString(), "[found=1, fixed=" +
(fixesPerEntry != null ? fixesPerEntry.toString() : ""));
assertEquals(EXIT_CODE_OK, execute("--cache", "idle_verify"));
- brokenParts.decrementAndGet();
+ if (fixesPerEntry != null)
+ if (fixesPerEntry > 0) {
+ brokenParts.decrementAndGet();
- if (brokenParts.get() > 0)
- assertContains(log, testOut.toString(),
- "conflict partitions has been found: [counterConflicts=0,
hashConflicts=" + brokenParts);
- else
- assertContains(log, testOut.toString(), "no conflicts have
been found");
+ if (brokenParts.get() > 0)
+ assertContains(log, testOut.toString(),
+ "conflict partitions has been found:
[counterConflicts=0, hashConflicts=" + brokenParts);
+ else
+ assertContains(log, testOut.toString(), "no conflicts
have been found");
+ }
+ else
+ assertContains(log, testOut.toString(),
+ "conflict partitions has been found:
[counterConflicts=0, hashConflicts=" + brokenParts); // Nothing fixed.
}
}
/**
*
*/
- private void readRepaitAtomic(AtomicInteger brokenParts, String cacheName)
{
- for (int i = 0; i < PARTITIONS; i++) { // This may be a copy of
previous (tx case), implement atomic repair to make this happen :)
- assertEquals(EXIT_CODE_OK, execute("--consistency", "repair",
cacheName, String.valueOf(i)));
- assertContains(log, testOut.toString(),
CONSISTENCY_VIOLATIONS_FOUND);
- assertContains(log, testOut.toString(), "[found=1, fixed=0"); //
Nothing fixed.
+ private Integer fixesPerEntry() {
+ switch (strategy) {
+ case PRIMARY:
+ case REMOVE:
+ return 1;
- assertEquals(EXIT_CODE_OK, execute("--cache", "idle_verify"));
- assertContains(log, testOut.toString(),
- "conflict partitions has been found: [counterConflicts=0,
hashConflicts=" + brokenParts); // Nothing fixed.
+ case CHECK_ONLY:
+ return 0;
+
+ case MAJORITY:
+ case LWW:
+ return null; // Who knows :)
Review comment:
I think a test has to be written in the such way that it actually knows
what exactly should be fixed.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheAdapter.java
##########
@@ -4833,6 +4835,9 @@ private boolean clearLocally0(K key, boolean readers) {
deserializeBinary,
needVer);
}
+ catch (IgniteIrreparableConsistencyViolationException e) {
+ throw e;
+ }
catch (IgniteConsistencyViolationException e) {
repairAsync(key, ctx.operationContextPerCall(), false).get();
Review comment:
IMHO, recursion without a limit param is always a bad solution. It looks
ok that if `repairAsync` is successful then `repairableGet` will be successfull
too. But in one moment due to some bug we can get a StackOverflowException here.
##########
File path:
modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerConsistencyTest.java
##########
@@ -187,36 +215,48 @@ public void testRepairNonExistentCache() throws Exception
{
/**
*
*/
- private void readRepairTx(AtomicInteger brokenParts, String cacheName) {
+ private void readRepair(AtomicInteger brokenParts, String cacheName,
Integer fixesPerEntry) {
for (int i = 0; i < PARTITIONS; i++) {
- assertEquals(EXIT_CODE_OK, execute("--consistency", "repair",
cacheName, String.valueOf(i)));
+ assertEquals(EXIT_CODE_OK, execute("--consistency", "repair",
cacheName, String.valueOf(i), strategy.toString()));
assertContains(log, testOut.toString(),
CONSISTENCY_VIOLATIONS_FOUND);
- assertContains(log, testOut.toString(), "[found=1, fixed=1");
+ assertContains(log, testOut.toString(), "[found=1, fixed=" +
(fixesPerEntry != null ? fixesPerEntry.toString() : ""));
assertEquals(EXIT_CODE_OK, execute("--cache", "idle_verify"));
- brokenParts.decrementAndGet();
+ if (fixesPerEntry != null)
+ if (fixesPerEntry > 0) {
+ brokenParts.decrementAndGet();
- if (brokenParts.get() > 0)
- assertContains(log, testOut.toString(),
- "conflict partitions has been found: [counterConflicts=0,
hashConflicts=" + brokenParts);
- else
- assertContains(log, testOut.toString(), "no conflicts have
been found");
+ if (brokenParts.get() > 0)
+ assertContains(log, testOut.toString(),
+ "conflict partitions has been found:
[counterConflicts=0, hashConflicts=" + brokenParts);
+ else
+ assertContains(log, testOut.toString(), "no conflicts
have been found");
+ }
+ else
+ assertContains(log, testOut.toString(),
+ "conflict partitions has been found:
[counterConflicts=0, hashConflicts=" + brokenParts); // Nothing fixed.
}
}
/**
*
*/
- private void readRepaitAtomic(AtomicInteger brokenParts, String cacheName)
{
- for (int i = 0; i < PARTITIONS; i++) { // This may be a copy of
previous (tx case), implement atomic repair to make this happen :)
- assertEquals(EXIT_CODE_OK, execute("--consistency", "repair",
cacheName, String.valueOf(i)));
- assertContains(log, testOut.toString(),
CONSISTENCY_VIOLATIONS_FOUND);
- assertContains(log, testOut.toString(), "[found=1, fixed=0"); //
Nothing fixed.
+ private Integer fixesPerEntry() {
+ switch (strategy) {
+ case PRIMARY:
+ case REMOVE:
+ return 1;
- assertEquals(EXIT_CODE_OK, execute("--cache", "idle_verify"));
- assertContains(log, testOut.toString(),
- "conflict partitions has been found: [counterConflicts=0,
hashConflicts=" + brokenParts); // Nothing fixed.
+ case CHECK_ONLY:
+ return 0;
+
+ case MAJORITY:
+ case LWW:
+ return null; // Who knows :)
+
+ default:
+ throw new UnsupportedOperationException("Unsupported trategy");
Review comment:
s/trategy/strategy/
##########
File path:
modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerConsistencyTest.java
##########
@@ -187,36 +215,48 @@ public void testRepairNonExistentCache() throws Exception
{
/**
*
*/
- private void readRepairTx(AtomicInteger brokenParts, String cacheName) {
+ private void readRepair(AtomicInteger brokenParts, String cacheName,
Integer fixesPerEntry) {
for (int i = 0; i < PARTITIONS; i++) {
- assertEquals(EXIT_CODE_OK, execute("--consistency", "repair",
cacheName, String.valueOf(i)));
+ assertEquals(EXIT_CODE_OK, execute("--consistency", "repair",
cacheName, String.valueOf(i), strategy.toString()));
assertContains(log, testOut.toString(),
CONSISTENCY_VIOLATIONS_FOUND);
- assertContains(log, testOut.toString(), "[found=1, fixed=1");
+ assertContains(log, testOut.toString(), "[found=1, fixed=" +
(fixesPerEntry != null ? fixesPerEntry.toString() : ""));
assertEquals(EXIT_CODE_OK, execute("--cache", "idle_verify"));
- brokenParts.decrementAndGet();
+ if (fixesPerEntry != null)
Review comment:
Actually there is no any check in `testAtomicAndTx` and
`testCacheFilter` for MAJORITY/LWW strategy. If `fixesPerEntry` equals to
`null`, there is no checks at all, except of single run of the command without
additional validations.
##########
File path: modules/core/src/main/java/org/apache/ignite/IgniteCache.java
##########
@@ -188,6 +189,19 @@
* <li>{@link IgniteCache#get} && {@link IgniteCache#getAsync}</li>
* <li>{@link IgniteCache#getAll} && {@link IgniteCache#getAllAsync}</li>
* </ul>
+ * @param strategy Read Repair strategy.
+ * @return Cache with explicit consistency check on each read and repair
if necessary.
+ */
+ @IgniteExperimental
+ public IgniteCache<K, V> withReadRepair(ReadRepairStrategy strategy);
Review comment:
AFAIU, `withReadRepair()` is low-level API, that assumed to be used by
external utils for specific cases with big care. So, I don't like the idea of
"default" strategy. Also, it's just an experimental API, so i think we should
make it as narrow as possible.
Let's enforce user to use this API with strict understanding what does
he/she want to fix and how, then `strategy` will be a required parameter. Also
we can write in javadocs some info like "In most cases the strategy LWW should
be OK, but check if it serves your needs."
But this API is still experimental, so default behavior can be changed in
the future after users actually starts use it. And it will be impossible to
change the hardcoded default. But instead we can change javadocs only.
WDYT?
##########
File path: modules/core/src/main/java/org/apache/ignite/IgniteCache.java
##########
@@ -188,6 +189,19 @@
* <li>{@link IgniteCache#get} && {@link IgniteCache#getAsync}</li>
* <li>{@link IgniteCache#getAll} && {@link IgniteCache#getAllAsync}</li>
* </ul>
+ * @param strategy Read Repair strategy.
+ * @return Cache with explicit consistency check on each read and repair
if necessary.
+ */
+ @IgniteExperimental
+ public IgniteCache<K, V> withReadRepair(ReadRepairStrategy strategy);
+
+ /**
+ * <b>This is an experimental API.</b>
+ * <p>
+ * Gets an instance of {@code IgniteCache} that will perform backup nodes
check on each get attempt with default
+ * conflict resolve strategy.
+ *
+ * @see IgniteCache#withReadRepair(ReadRepairStrategy) for defails.
Review comment:
s/defails/details/
##########
File path:
modules/core/src/main/java/org/apache/ignite/events/CacheConsistencyViolationEvent.java
##########
@@ -67,39 +68,58 @@
private static final long serialVersionUID = 0L;
/** Represents original values of entries.*/
- final Map<Object, Map<ClusterNode, EntryInfo>> entries;
+ final Map<?, Map<ClusterNode, EntryInfo>> entries;
Review comment:
But `GridNearReadRepairAbstractFuture` creates this event with the
`Object` class in generics for both `entries` and `fixed`. Why do we need this
change?
##########
File path: docs/_docs/tools/control-script.adoc
##########
@@ -1052,6 +1053,7 @@ Parameters:
| Parameter | Description
| `cache-name`| Cache to be checked/repaired..
Review comment:
Double dot in the end of line.
##########
File path:
modules/core/src/main/java/org/apache/ignite/cache/ReadRepairStrategy.java
##########
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.cache;
+
+import org.apache.ignite.IgniteCache;
+
+/**
+ * Read repair strategies.
Review comment:
Let's add some more docs here. I think we should write here about known
trade-offs of different methods, or write that actually they have similar
performance.
##########
File path:
modules/core/src/main/java/org/apache/ignite/events/CacheConsistencyViolationEvent.java
##########
@@ -67,39 +68,58 @@
private static final long serialVersionUID = 0L;
/** Represents original values of entries.*/
- final Map<Object, Map<ClusterNode, EntryInfo>> entries;
+ final Map<?, Map<ClusterNode, EntryInfo>> entries;
+
+ /** Fixed entries. */
+ final Map<?, ?> fixed;
/** Cache name. */
final String cacheName;
+ /** Strategy. */
+ final ReadRepairStrategy strategy;
Review comment:
let's make all fields `private`
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheOperationContext.java
##########
@@ -50,7 +51,7 @@
private final boolean recovery;
/** Read-repair flag. */
Review comment:
Fix javadoc here too
##########
File path:
modules/core/src/main/java/org/apache/ignite/events/CacheConsistencyViolationEvent.java
##########
@@ -67,39 +68,58 @@
private static final long serialVersionUID = 0L;
/** Represents original values of entries.*/
- final Map<Object, Map<ClusterNode, EntryInfo>> entries;
+ final Map<?, Map<ClusterNode, EntryInfo>> entries;
+
+ /** Fixed entries. */
+ final Map<?, ?> fixed;
/** Cache name. */
final String cacheName;
+ /** Strategy. */
+ final ReadRepairStrategy strategy;
+
/**
* Creates a new instance of CacheConsistencyViolationEvent.
- *
- * @param cacheName Cache name.
+ * @param cacheName Cache name.
Review comment:
Please, remove the whitespace before `@param`
##########
File path:
modules/core/src/main/java/org/apache/ignite/events/CacheConsistencyViolationEvent.java
##########
@@ -67,39 +68,58 @@
private static final long serialVersionUID = 0L;
/** Represents original values of entries.*/
- final Map<Object, Map<ClusterNode, EntryInfo>> entries;
+ final Map<?, Map<ClusterNode, EntryInfo>> entries;
+
+ /** Fixed entries. */
+ final Map<?, ?> fixed;
/** Cache name. */
final String cacheName;
+ /** Strategy. */
+ final ReadRepairStrategy strategy;
+
/**
* Creates a new instance of CacheConsistencyViolationEvent.
- *
- * @param cacheName Cache name.
+ * @param cacheName Cache name.
* @param node Local node.
* @param msg Event message.
* @param entries Collection of original entries.
+ * @param fixed Collection of fixed entries.
+ * @param strategy
*/
public CacheConsistencyViolationEvent(
String cacheName,
ClusterNode node,
String msg,
- Map<Object, Map<ClusterNode, EntryInfo>> entries) {
+ Map<?, Map<ClusterNode, EntryInfo>> entries,
+ Map<?, ?> fixed, ReadRepairStrategy strategy) {
Review comment:
Move the `strategy` definition on new line.
##########
File path:
modules/core/src/main/java/org/apache/ignite/events/CacheConsistencyViolationEvent.java
##########
@@ -67,39 +68,58 @@
private static final long serialVersionUID = 0L;
/** Represents original values of entries.*/
- final Map<Object, Map<ClusterNode, EntryInfo>> entries;
+ final Map<?, Map<ClusterNode, EntryInfo>> entries;
+
+ /** Fixed entries. */
+ final Map<?, ?> fixed;
/** Cache name. */
final String cacheName;
+ /** Strategy. */
+ final ReadRepairStrategy strategy;
+
/**
* Creates a new instance of CacheConsistencyViolationEvent.
- *
- * @param cacheName Cache name.
+ * @param cacheName Cache name.
* @param node Local node.
* @param msg Event message.
* @param entries Collection of original entries.
+ * @param fixed Collection of fixed entries.
+ * @param strategy
Review comment:
Add a short doc with a dot in the end of line.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/CacheOperationContext.java
##########
@@ -92,7 +93,7 @@ public CacheOperationContext(
boolean noRetries,
@Nullable Byte dataCenterId,
boolean recovery,
- boolean readRepair,
+ ReadRepairStrategy readRepairStrategy,
Review comment:
Please mark it with `@Nullable`
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProxyImpl.java
##########
@@ -294,7 +294,7 @@ public GridCacheProxyImpl(
false,
null,
false,
- false,
+ null,
Review comment:
Here and below, let's replace it with
`CacheOperationContext.keepBinary()`
##########
File path:
modules/core/src/main/java/org/apache/ignite/events/CacheConsistencyViolationEvent.java
##########
@@ -67,39 +68,58 @@
private static final long serialVersionUID = 0L;
/** Represents original values of entries.*/
- final Map<Object, Map<ClusterNode, EntryInfo>> entries;
+ final Map<?, Map<ClusterNode, EntryInfo>> entries;
+
+ /** Fixed entries. */
+ final Map<?, ?> fixed;
/** Cache name. */
final String cacheName;
+ /** Strategy. */
+ final ReadRepairStrategy strategy;
+
/**
* Creates a new instance of CacheConsistencyViolationEvent.
- *
- * @param cacheName Cache name.
+ * @param cacheName Cache name.
* @param node Local node.
* @param msg Event message.
* @param entries Collection of original entries.
+ * @param fixed Collection of fixed entries.
+ * @param strategy
*/
public CacheConsistencyViolationEvent(
String cacheName,
ClusterNode node,
String msg,
- Map<Object, Map<ClusterNode, EntryInfo>> entries) {
+ Map<?, Map<ClusterNode, EntryInfo>> entries,
+ Map<?, ?> fixed, ReadRepairStrategy strategy) {
super(node, msg, EVT_CONSISTENCY_VIOLATION);
this.cacheName = cacheName;
this.entries = entries;
+ this.fixed = fixed;
+ this.strategy = strategy;
}
/**
* Returns a mapping of keys to a collection of original entries.
*
* @return Collection of original entries.
*/
- public Map<Object, Map<ClusterNode, EntryInfo>> getEntries() {
+ public Map<?, Map<ClusterNode, EntryInfo>> getEntries() {
Review comment:
Usage of this method applies `Object key` for accessing entries. Why do
we need this change?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
