[GitHub] [ignite] alex-plekhanov commented on a change in pull request #7924: IGNITE-13144: minor usability and code improvements in a cluster read-only mode.

[GitBox <gi...@apache.org>]

alex-plekhanov commented on a change in pull request #7924:
URL: https://github.com/apache/ignite/pull/7924#discussion_r442817465



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cluster/GridClusterStateProcessor.java
##########
@@ -924,14 +901,11 @@ protected IgniteCheckedException concurrentStateChangeError(ClusterState state,
                 throw new IgniteException("Node with BaselineTopology cannot join" +
                     " mixed cluster running in compatibility mode");
 
-            globalState = (DiscoveryDataClusterState) data.commonData();
+            globalState = (DiscoveryDataClusterState)data.commonData();
 
             compatibilityMode = true;
 
-            ctx.cache().context().readOnlyMode(readOnly(globalState.state()));
-
-            if (readOnly(globalState.state()))
-                ctx.cache().context().database().forceCheckpoint("Cluster read-only mode enabled");
+            ctx.cache().context().readOnlyMode(globalState.state() == ACTIVE_READ_ONLY);

Review comment:
       Why do we don't need `forceCheckpoint` anymore?

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/StateChangeRequest.java
##########
@@ -111,7 +109,7 @@ public ClusterState prevState() {
      * @return {@code True} if active state was changed.
      */
     public boolean activeChanged() {
-        return active(prevState) && !active(msg.state()) || !active(prevState) && active(msg.state());
+        return prevState.active() ^ msg.state().active();

Review comment:
       I think `prevState.active() != msg.state().active()` is more readable. WDYT?

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cluster/GridClusterStateProcessor.java
##########
@@ -1821,6 +1792,36 @@ private boolean hasInMemoryCache() {
                 && (!desc.cacheConfiguration().isWriteBehindEnabled() || !desc.cacheConfiguration().isReadThrough()));
     }
 
+    /**
+     * Gets state of given two with minimal number of features.
+     * <p/>
+     * The order: {@link ClusterState#ACTIVE} > {@link ClusterState#ACTIVE_READ_ONLY} > {@link ClusterState#INACTIVE}.
+     * <p/>
+     * Explain:
+     * <br/>
+     * {@link ClusterState#INACTIVE} has the smallast number of available features. You can't use caches in this state.
+     * <br/>
+     * In {@link ClusterState#ACTIVE_READ_ONLY} you have more available features than {@link ClusterState#INACTIVE} state,
+     * such as reading from caches, but can't write into them.
+     * <br/> In {@link ClusterState#ACTIVE} you can update caches. It's a state with the biggest number of features.
+     *
+     * @param state1 First given state.
+     * @param state2 Second given state.
+     * @return State with minimal number of available features.
+     */
+    public static ClusterState withMinimalFeatures(ClusterState state1, ClusterState state2) {

Review comment:
       Naming looks confusing. It's hard to guess what method will return without Javadoc. Also, `with` prefix used in Ignite to return instance with some modificator (withKeepBinary, withNoFailover, etc). Please, consider something like `stateWithMinimalFeatures` or just `minState`.

##########
File path: modules/clients/src/test/java/org/apache/ignite/internal/processors/rest/ChangeStateCommandHandlerTest.java
##########
@@ -90,9 +89,7 @@
     public void testActivateDeActivate() throws GridClientException {
         GridClientClusterState state = client.state();
 
-        boolean active = active(state.state());
-
-        assertTrue(active);
+        assertTrue(state.toString(), state.state().active());

Review comment:
       `toString()` is not overriden in `GridClientClusterState`. I think it's better to use here `state.state().toString()`




----------------------------------------------------------------
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