[GitHub] [ignite] Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST.
Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow
silent deactivation in CLI and REST.
URL: https://github.com/apache/ignite/pull/7471#discussion_r389878598
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cluster/GridClusterStateProcessor.java
##
@@ -1617,6 +1643,7 @@ public ExchangeActions
autoAdjustExchangeActions(ExchangeActions exchActs) {
ctx.localNodeId(),
null,
ClusterState.active(clusterState.state()) ?
clusterState.state() : ACTIVE,
+true,
Review comment:
> It's not clear why we force deactivation here.
All previous internal changes of cluster state were forced. Thus, all
current internal changes of cluster are considered forced. Only the exceptions
are the user commands being translated to not-forced internal calls without
flag ‘force’ passed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST.
Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST. URL: https://github.com/apache/ignite/pull/7471#discussion_r387548787 ## File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cluster/ChangeGlobalStateMessage.java ## @@ -71,6 +71,9 @@ @GridToStringExclude @Nullable private transient ServiceDeploymentActions serviceDeploymentActions; +/** Forced change of cluster state. */ +private boolean force = true; Review comment: > Why the default value is true? For backward compatibility and due to general idea. Within this ticket we prevent unsafe deactivation only where required. Most of the deactivation requests are still forced. Like through `Ignite, IgniteMXBean, IgniteCluster` etc. Also there is usage of this message from another places: `GridClusterStateProcessor#autoAdjustExchangeActions(), GridClusterStateProcessor#autoAdjustInMemoryClusterState()` 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST.
Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST. URL: https://github.com/apache/ignite/pull/7471#discussion_r387548787 ## File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cluster/ChangeGlobalStateMessage.java ## @@ -71,6 +71,9 @@ @GridToStringExclude @Nullable private transient ServiceDeploymentActions serviceDeploymentActions; +/** Forced change of cluster state. */ +private boolean force = true; Review comment: > Why the default value is true? For backward compatibility. Most of the deactivation requests are still forced. Like through `Ignite, IgniteMXBean, IgniteCluster` etc. Also there is usage of this message from another places: `GridClusterStateProcessor#autoAdjustExchangeActions(), GridClusterStateProcessor#autoAdjustInMemoryClusterState()` 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST.
Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow
silent deactivation in CLI and REST.
URL: https://github.com/apache/ignite/pull/7471#discussion_r387563217
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cluster/GridClusterStateProcessor.java
##
@@ -924,49 +945,8 @@ protected IgniteCheckedException
concurrentStateChangeError(ClusterState state,
/** {@inheritDoc} */
@Override public IgniteInternalFuture changeGlobalState(
-final boolean activate,
Review comment:
> Why this deletion?
To simplify and reduce internal API and code base. There were already 2
changeGlobalState(). First, I introduced new one with ‘force’ not to change
previous API. There 3 methods (2 of them deprecated) became a mesh. After all,
as it is internal API, I suggest to keep one change-state method. Isn’t it
better?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST.
Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow
silent deactivation in CLI and REST.
URL: https://github.com/apache/ignite/pull/7471#discussion_r387568450
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/rest/client/message/GridClientClusterStateRequest.java
##
@@ -69,6 +70,21 @@ public static GridClientClusterStateRequest
state(ClusterState state) {
return msg;
}
+/** Default constructor for the exernalization. */
Review comment:
Fixed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST.
Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST. URL: https://github.com/apache/ignite/pull/7471#discussion_r387568120 ## File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cluster/GridClusterStateProcessor.java ## @@ -115,6 +118,11 @@ /** */ private static final String METASTORE_CURR_BLT_KEY = "metastoreBltKey"; +/** Warning of unsafe deactivation. */ +public static final String DATA_LOST_ON_DEACTIVATION_WARNING = +"Cluster has caches configured without persistence. " + +"During deactivation in-memory data and objects can be lost!"; Review comment: Fixed. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST.
Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow
silent deactivation in CLI and REST.
URL: https://github.com/apache/ignite/pull/7471#discussion_r387566134
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cluster/IGridClusterStateProcessor.java
##
@@ -108,30 +108,20 @@ boolean onStateChangeMessage(AffinityTopologyVersion
topVer,
/** */
void cacheProcessorStarted();
-/**
- * @param activate New cluster state.
- * @param baselineNodes New baseline nodes.
- * @param forceChangeBaselineTopology Force change baseline topology.
- * @return State change future.
- * @deprecated Use {@link #changeGlobalState(ClusterState, Collection,
boolean)} instead.
- */
-@Deprecated
-IgniteInternalFuture changeGlobalState(
-boolean activate,
-Collection baselineNodes,
-boolean forceChangeBaselineTopology
-);
-
/**
* @param state New cluster state.
+ * @param force If {@code True} skips checking of the operation safety.
Review comment:
Fixed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST.
Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow
silent deactivation in CLI and REST.
URL: https://github.com/apache/ignite/pull/7471#discussion_r387566298
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cluster/GridClusterStateProcessor.java
##
@@ -924,49 +945,8 @@ protected IgniteCheckedException
concurrentStateChangeError(ClusterState state,
/** {@inheritDoc} */
@Override public IgniteInternalFuture changeGlobalState(
-final boolean activate,
-Collection baselineNodes,
-boolean forceChangeBaselineTopology
-) {
-return changeGlobalState(activate, baselineNodes,
forceChangeBaselineTopology, false);
-}
-
-/** {@inheritDoc} */
-@Override public IgniteInternalFuture changeGlobalState(
-ClusterState state,
-Collection baselineNodes,
-boolean forceChangeBaselineTopology
-) {
-return changeGlobalState(state, baselineNodes,
forceChangeBaselineTopology, false);
-}
-
-/**
- * @param activate New activate state.
- * @param baselineNodes New BLT nodes.
- * @param forceChangeBaselineTopology Force change BLT.
- * @param isAutoAdjust Auto adjusting flag.
- * @return Global change state future.
- * @deprecated Use {@link #changeGlobalState(ClusterState, Collection,
boolean, boolean)} instead.
- */
-@Deprecated
-public IgniteInternalFuture changeGlobalState(
-final boolean activate,
-Collection baselineNodes,
-boolean forceChangeBaselineTopology,
-boolean isAutoAdjust
-) {
-return changeGlobalState(activate ? ACTIVE : INACTIVE, baselineNodes,
forceChangeBaselineTopology, isAutoAdjust);
-}
-
-/**
- * @param state New activate state.
- * @param baselineNodes New BLT nodes.
- * @param forceChangeBaselineTopology Force change BLT.
- * @param isAutoAdjust Auto adjusting flag.
- * @return Global change state future.
- */
-public IgniteInternalFuture changeGlobalState(
ClusterState state,
+boolean force,
Review comment:
Fixed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST.
Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow
silent deactivation in CLI and REST.
URL: https://github.com/apache/ignite/pull/7471#discussion_r387566194
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cluster/GridClusterStateProcessor.java
##
@@ -1045,6 +1025,7 @@ else if (ClusterState.active(state)) {
/** */
private IgniteInternalFuture changeGlobalState0(
ClusterState state,
+boolean force,
Review comment:
Fixed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST.
Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow
silent deactivation in CLI and REST.
URL: https://github.com/apache/ignite/pull/7471#discussion_r387564676
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/rest/client/message/GridClientClusterStateRequestV2.java
##
@@ -0,0 +1,83 @@
+/*
+ * 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.internal.processors.rest.client.message;
+
+import java.io.IOException;
+import java.io.ObjectInput;
+import java.io.ObjectOutput;
+import org.apache.ignite.cluster.ClusterState;
+
+/**
+ * Enchanced version of {@link GridClientClusterStateRequest}.
+ * Introduced to support forced version of the change state command and keep
backward compatibility
+ * with nodes of old version that may occur in cluster at the rolling updates.
+ */
+public class GridClientClusterStateRequestV2 extends
GridClientClusterStateRequest {
+/** */
+private static final long serialVersionUID = 0L;
+
+/** Forced change of cluster state. */
+private boolean force;
Review comment:
Fixed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST.
Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow
silent deactivation in CLI and REST.
URL: https://github.com/apache/ignite/pull/7471#discussion_r387563805
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cluster/IGridClusterStateProcessor.java
##
@@ -108,30 +108,20 @@ boolean onStateChangeMessage(AffinityTopologyVersion
topVer,
/** */
void cacheProcessorStarted();
-/**
- * @param activate New cluster state.
- * @param baselineNodes New baseline nodes.
- * @param forceChangeBaselineTopology Force change baseline topology.
- * @return State change future.
- * @deprecated Use {@link #changeGlobalState(ClusterState, Collection,
boolean)} instead.
- */
-@Deprecated
-IgniteInternalFuture changeGlobalState(
Review comment:
> Why this deletion?
Same as above ("To simplify and reduce internal API and code base...")
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST.
Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow
silent deactivation in CLI and REST.
URL: https://github.com/apache/ignite/pull/7471#discussion_r387563217
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cluster/GridClusterStateProcessor.java
##
@@ -924,49 +945,8 @@ protected IgniteCheckedException
concurrentStateChangeError(ClusterState state,
/** {@inheritDoc} */
@Override public IgniteInternalFuture changeGlobalState(
-final boolean activate,
Review comment:
> Why this deletion?
To simplify and reduce internal API and code base. There were already 2
changeGlobalState(). First, I introduced new one with ‘force’ not to change
previous API. There 3 (2 of them deprecated) became a mesh. After all, as it is
internal API, I suggest to keep one change-state method. Isn’t it better?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST.
Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST. URL: https://github.com/apache/ignite/pull/7471#discussion_r387548787 ## File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cluster/ChangeGlobalStateMessage.java ## @@ -71,6 +71,9 @@ @GridToStringExclude @Nullable private transient ServiceDeploymentActions serviceDeploymentActions; +/** Forced change of cluster state. */ +private boolean force = true; Review comment: > Why the default value is true? For backward compatibility. Most of the deactivation requests are still forcable. Through Ignite, IgniteMXBean, IgniteCluster etc. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST.
Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow
silent deactivation in CLI and REST.
URL: https://github.com/apache/ignite/pull/7471#discussion_r387547266
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/commandline/DeactivateCommand.java
##
@@ -78,6 +81,21 @@
return null;
}
+/** {@inheritDoc} */
+@Override public void parseArguments(CommandArgIterator argIter) {
+force = false;
Review comment:
> Do we need this line? Default `force` value is `false` already.
Yes, we need this. Commands are statefull unfortunately. Without this, next
command usage will go with previous value 'force' param.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow silent deactivation in CLI and REST.
Vladsz83 commented on a change in pull request #7471: IGNITE-12701 : Disallow
silent deactivation in CLI and REST.
URL: https://github.com/apache/ignite/pull/7471#discussion_r387547117
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/commandline/ClusterStateChangeCommand.java
##
@@ -91,6 +98,18 @@
catch (IllegalArgumentException e) {
throw new IllegalArgumentException("Can't parse new cluster state.
State: " + s, e);
}
+
+force = false;
Review comment:
> Do we need this line? Default `force` value is `false` already.
Yes, we need this. Commands are statefull unfortunately. Without this, next
command usage will go with previous value 'force' param.
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:
us...@infra.apache.org
With regards,
Apache Git Services
