denis-chudov commented on code in PR #5685:
URL: https://github.com/apache/ignite-3/pull/5685#discussion_r2068983864
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/storage/CatalogSerializationCompatibilityTest.java:
##########
@@ -393,7 +400,10 @@ private <T extends UpdateLogEvent> T checkEntry(Class<T>
entryClass, String entr
log.info("Read fileName: {}, class: {}, version: {}", fileName,
entryClass.getSimpleName(), version);
- UpdateLogMarshallerImpl marshaller = new
UpdateLogMarshallerImpl(provider, protocolVersion());
+ UpdateLogMarshaller marshaller = new UpdateLogMarshallerImpl(provider,
protocolVersion());
+
+ // Uncomment this and run tests with corresponding entryVersion to
write entry to file
+ // writeEntry(entry, resourceName, marshaller);
Review Comment:
Did you leave it intentionally?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AlterZoneCommand.java:
##########
@@ -136,15 +142,20 @@ private CatalogZoneDescriptor
fromParamsAndPreviousValue(CatalogZoneDescriptor p
CatalogStorageProfilesDescriptor storageProfiles =
storageProfileParams != null
? fromParams(storageProfileParams) :
previous.storageProfiles();
+ // Validate quorum size here since its boundaries depend on the
current number of replicas
+ int replicas = requireNonNullElse(this.replicas, previous.replicas());
+ validateQuorum(quorumSize, replicas);
Review Comment:
The validation error message in the case of replicas count alteration is not
very user-friendly. It's not obvious that sometimes the user should change the
quorum size along with replicas count. Could you please make it more verbose?
Also, the test checking replicas count change
`AlterZoneCommandValidationTest#zoneReplicas` doesn't check the consistency of
new replicas count with quorum size
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogZoneDescriptor.java:
##########
@@ -151,6 +160,20 @@ public int replicas() {
return replicas;
}
+ /**
+ * Return quorum size.
+ */
Review Comment:
Let's add more context somewhere, what quorum size is and how it is related
to consensus group size (i.e. voters count) and total replicas count.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogZoneDescriptor.java:
##########
@@ -151,6 +160,20 @@ public int replicas() {
return replicas;
}
+ /**
+ * Return quorum size.
+ */
+ public int quorumSize() {
+ return quorumSize;
+ }
+
+ /**
+ * Return consensus group size.
+ */
Review Comment:
Let's also describe what consensus group is.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogParamsValidationUtils.java:
##########
@@ -142,6 +146,18 @@ public static void validatePartition(@Nullable Integer
partitions) {
}
}
+ /**
+ * Validates quorum size, taking number of replicas into consideration.
+ *
+ * @param quorumSize Quorum size to validate.
+ * @param replicas Current number of replicas.
+ */
+ public static void validateQuorum(@Nullable Integer quorumSize, int
replicas) {
+ int minQuorum = min(replicas, 2);
+ int maxQuorum = max(minQuorum, (int) (floor(replicas / 2.0 + 0.5)));
Review Comment:
why not simply `round()` instead of `floor()` with `+ 0.5`?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogZoneDescriptorSerializers.java:
##########
@@ -149,6 +153,7 @@ public void writeTo(CatalogZoneDescriptor descriptor,
CatalogObjectDataOutput ou
output.writeVarInt(descriptor.partitions());
output.writeVarInt(descriptor.replicas());
+ output.writeVarInt(descriptor.quorumSize());
Review Comment:
I will leave this comment as a placeholder; we need to find out whether we
still can modify these serializers with consideration of nearest release
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AlterZoneCommandBuilder.java:
##########
@@ -46,6 +46,14 @@ public interface AlterZoneCommandBuilder extends
AbstractZoneCommandBuilder<Alte
*/
AlterZoneCommandBuilder replicas(@Nullable Integer replicas);
+ /**
+ * Sets the quorum size.
+ *
+ * @param quorumSize Optional quorum size, it should be in the range from
1 to the {@code Math.floor((replicas + 1)/2) }.
Review Comment:
This javadoc doesn't describe the range accurately
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -61,6 +62,9 @@ public class CatalogUtils {
/** Default number of distribution zone replicas. */
public static final int DEFAULT_REPLICA_COUNT = 1;
+ /** Default quorum size. */
Review Comment:
```suggestion
/**
* Default quorum size. This is used only to initialize the default
zone; in other cases the default quorum size is calculated using
* replicas count.
*/
```
--
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
