rpuch commented on code in PR #4547:
URL: https://github.com/apache/ignite-3/pull/4547#discussion_r1801206985
##########
modules/system-disaster-recovery/src/main/java/org/apache/ignite/internal/disaster/system/SystemDisasterRecoveryManagerImpl.java:
##########
@@ -217,19 +226,24 @@ private CompletableFuture<Void> resetClusterInternal(
}
}
- private CompletableFuture<Void> doResetCluster(List<String>
proposedCmgNodeNames, @Nullable Integer metastorageReplicationFactor) {
- ensureReplicationFactorIsPositiveIfGiven(metastorageReplicationFactor);
+ private CompletableFuture<Void> doResetCluster(
+ Collection<String> proposedCmgNodeNames,
+ @Nullable Integer metastorageReplicationFactor
+ ) {
+ Collection<ClusterNode> nodesInTopology = topologyService.allMembers();
- ensureNoRepetitions(proposedCmgNodeNames);
- ensureContainsThisNodeName(proposedCmgNodeNames);
+ if (proposedCmgNodeNames != null) {
Review Comment:
It can't be `null`, so the null check seems to be excessive
##########
modules/rest/src/main/java/org/apache/ignite/internal/rest/recovery/system/SystemDisasterRecoveryController.java:
##########
@@ -61,7 +61,14 @@ public
SystemDisasterRecoveryController(SystemDisasterRecoveryManager systemDisa
public CompletableFuture<Void> reset(ResetClusterRequest command) {
LOG.info("Reset command is {}", command);
- return
systemDisasterRecoveryManager.resetCluster(command.cmgNodeNames());
+ if (command.metastorageReplicationFactor() == null) {
Review Comment:
Let's introduce a method `metastorageRepairRequested()` to
`ResetClusterRequest` and use it here
##########
modules/rest-api/src/main/java/org/apache/ignite/internal/rest/api/recovery/system/ResetClusterRequest.java:
##########
@@ -25,28 +25,48 @@
import java.util.Objects;
import org.apache.ignite.internal.tostring.IgniteToStringInclude;
import org.apache.ignite.internal.tostring.S;
+import org.jetbrains.annotations.Nullable;
/** Request to reset cluster. */
@Schema(description = "Reset cluster.")
public class ResetClusterRequest {
- @Schema(description = "Names of the proposed CMG nodes.")
+ @Schema(description = "Names of the proposed CMG nodes. Optional if
Metastorage replication factor is specified, then "
+ + "current CMG nodes will be used.")
@IgniteToStringInclude
- private final List<String> cmgNodeNames;
+ private final @Nullable List<String> cmgNodeNames;
+
+ @Schema(description = "Number of nodes in the Raft voting member set for
Metastorage.")
+ @IgniteToStringInclude
+ private final @Nullable Integer metastorageReplicationFactor;
/** Constructor. */
@JsonCreator
- public ResetClusterRequest(@JsonProperty("cmgNodeNames") List<String>
cmgNodeNames) {
- Objects.requireNonNull(cmgNodeNames);
+ public ResetClusterRequest(
+ @JsonProperty("cmgNodeNames") @Nullable List<String> cmgNodeNames,
+ @JsonProperty("metastorageReplicationFactor") @Nullable Integer
metastorageReplicationFactor
+ ) {
+ if (metastorageReplicationFactor == null) {
+ Objects.requireNonNull(cmgNodeNames);
+ this.cmgNodeNames = List.copyOf(cmgNodeNames);
+ } else {
+ this.cmgNodeNames = cmgNodeNames;
Review Comment:
Why don't we make a copy on this branch?
##########
modules/rest-api/src/main/java/org/apache/ignite/internal/rest/api/recovery/system/ResetClusterRequest.java:
##########
@@ -25,28 +25,48 @@
import java.util.Objects;
import org.apache.ignite.internal.tostring.IgniteToStringInclude;
import org.apache.ignite.internal.tostring.S;
+import org.jetbrains.annotations.Nullable;
/** Request to reset cluster. */
@Schema(description = "Reset cluster.")
public class ResetClusterRequest {
- @Schema(description = "Names of the proposed CMG nodes.")
+ @Schema(description = "Names of the proposed CMG nodes. Optional if
Metastorage replication factor is specified, then "
+ + "current CMG nodes will be used.")
@IgniteToStringInclude
- private final List<String> cmgNodeNames;
+ private final @Nullable List<String> cmgNodeNames;
+
+ @Schema(description = "Number of nodes in the Raft voting member set for
Metastorage.")
+ @IgniteToStringInclude
+ private final @Nullable Integer metastorageReplicationFactor;
/** Constructor. */
@JsonCreator
- public ResetClusterRequest(@JsonProperty("cmgNodeNames") List<String>
cmgNodeNames) {
- Objects.requireNonNull(cmgNodeNames);
+ public ResetClusterRequest(
+ @JsonProperty("cmgNodeNames") @Nullable List<String> cmgNodeNames,
+ @JsonProperty("metastorageReplicationFactor") @Nullable Integer
metastorageReplicationFactor
+ ) {
+ if (metastorageReplicationFactor == null) {
+ Objects.requireNonNull(cmgNodeNames);
Review Comment:
What will happen if the user does not send any of these fields? It seems
that this will result in an NPE. Let's remove this `requireNonNull()` and make
the validation in the manager (throwing a `ClusterResetException`)
--
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]
