IGNITE-28731 Create cluster auto activation plugin - #355
Conversation
e1ba8c9 to
426bd4b
Compare
| if (cluster.state() == ClusterState.ACTIVE) { | ||
| if (logger.isInfoEnabled()) | ||
| logger.info("Auto activation skipped - cluster already activated"); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (cluster.currentBaselineTopology() != null) { | ||
| if (logger.isInfoEnabled()) | ||
| logger.info("Auto activation skipped - baseline is not empty"); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (condition.apply(cluster.nodes())) { | ||
| if (logger.isInfoEnabled()) | ||
| logger.info("Auto activation plugin set cluster state ACTIVE - activation condition meet"); | ||
|
|
||
| cluster.state(ClusterState.ACTIVE); | ||
| } | ||
| else { | ||
| if (logger.isInfoEnabled()) | ||
| logger.info("Auto activation skipped - activation condition not meet"); | ||
| } |
There was a problem hiding this comment.
Should ACTIVE_READ_ONLY be treated as already activated here? With the current check the plugin skips only ACTIVE and later calls cluster.state(ACTIVE), so a read-only cluster can be promoted to read-write mode by auto-activation. I think auto-activation should only activate INACTIVE clusters and leave ACTIVE_READ_ONLY unchanged.
Also add test for ACTIVE_READ_ONLY nodes
There was a problem hiding this comment.
Absolutely agree. Added a check for ClusterState.ACTIVE_READ_ONLY and corresponding tests
| return; | ||
| } | ||
|
|
||
| if (condition.apply(cluster.nodes())) { |
There was a problem hiding this comment.
Should this condition be evaluated only against server nodes? cluster.nodes() includes client nodes, so a client node with a matching consistentId or attribute can satisfy the activation condition and trigger cluster activation before the required server topology is actually present
WDYT?
There was a problem hiding this comment.
Also add test for scenario with client nodes
There was a problem hiding this comment.
This is a fair point. I added throwing an 'IgniteException' when a client node is included in the 'conditions' for auto-activation and tests
| @@ -0,0 +1,113 @@ | |||
| <beans xmlns="http://www.springframework.org/schema/beans" | |||
There was a problem hiding this comment.
These XML test configs duplicate most of the Ignite configuration. Can we move the common storage/cache/discovery/connector settings to a shared parent bean or common XML file, and keep only node-specific values here?
0d6b493 to
115b23d
Compare
|
|
||
| /** {@inheritDoc} */ | ||
| @Override public boolean apply(Collection<ClusterNode> nodes) { | ||
| Set<String> missingNodes = new LinkedHashSet<>(requiredNodes); |
There was a problem hiding this comment.
| Set<String> missingNodes = new LinkedHashSet<>(requiredNodes); | |
| Set<String> missingNodes = new HashSet<>(requiredNodes); |
| if (condition.apply(cluster.nodes())) { | ||
| if (logger.isInfoEnabled()) | ||
| logger.info("Auto activation plugin set cluster state ACTIVE - activation condition meet"); | ||
|
|
||
| cluster.state(ClusterState.ACTIVE); | ||
| } | ||
| else { | ||
| if (logger.isInfoEnabled()) | ||
| logger.info("Auto activation skipped - activation condition not meet"); | ||
| } | ||
| } |
There was a problem hiding this comment.
| if (condition.apply(cluster.nodes())) { | |
| if (logger.isInfoEnabled()) | |
| logger.info("Auto activation plugin set cluster state ACTIVE - activation condition meet"); | |
| cluster.state(ClusterState.ACTIVE); | |
| } | |
| else { | |
| if (logger.isInfoEnabled()) | |
| logger.info("Auto activation skipped - activation condition not meet"); | |
| } | |
| } | |
| if (condition.apply(cluster.nodes())) { | |
| if (logger.isInfoEnabled()) | |
| logger.info("Auto activation plugin set cluster state ACTIVE - activation condition meet"); | |
| cluster.state(ClusterState.ACTIVE); | |
| return; | |
| } | |
| if (logger.isInfoEnabled()) | |
| logger.info("Auto activation skipped - activation condition not meet"); | |
| } |
There was a problem hiding this comment.
Usually it's better to use extra return instead of else
|
|
||
| /** {@inheritDoc} */ | ||
| @Override public String copyright() { | ||
| return ""; |
There was a problem hiding this comment.
| return ""; | |
| return "Apache Software Foundation"; |
There was a problem hiding this comment.
Same as in org.apache.ignite.cdc.conflictresolve.CacheVersionConflictResolverPluginProvider#copyright
|
|
||
| /** {@inheritDoc} */ | ||
| @Override public boolean apply(Collection<ClusterNode> nodes) { | ||
| Set<String> missingNodes = new LinkedHashSet<>(requiredValues); |
There was a problem hiding this comment.
| Set<String> missingNodes = new LinkedHashSet<>(requiredValues); | |
| Set<String> missingNodes = new HashSet<>(requiredValues); |
There was a problem hiding this comment.
Better to use HashSet when we do not need insertion order for iteration
| <!-- | ||
| POM file. | ||
| --> |
There was a problem hiding this comment.
| <!-- | |
| POM file. | |
| --> |
There was a problem hiding this comment.
Delete to fix Ignoring multiple XML header comment! warning for command mvn clean install -pl :ignite-auto-activation-ext -am -Pcheckstyle -DskipTests
| return; | ||
| } | ||
|
|
||
| if (condition.apply(cluster.nodes())) { |
There was a problem hiding this comment.
Can we use cluster.forServers().nodes() here? Then we do not need client checks in both predicate classes.
Or replace IgniteException("Auto-activation-plugin supports on with return false
| import org.apache.ignite.plugin.PluginValidationException; | ||
|
|
||
| /** | ||
| * Activate cluster when specified condition meet |
There was a problem hiding this comment.
| * Activate cluster when specified condition meet | |
| * Activate cluster when specified condition meet. |
| import static org.apache.ignite.testframework.GridTestUtils.assertThrows; | ||
|
|
||
| /** | ||
| * {@link AutoActivationPluginProvider} test |
There was a problem hiding this comment.
| * {@link AutoActivationPluginProvider} test | |
| * Tests {@link AutoActivationPluginProvider}. |
| - Cluster baseline is not empty | ||
| - `condition` contains any client node | ||
|
|
||
| Depending on how you use Ignite, you can an extension using one of the following methods: |
There was a problem hiding this comment.
you can an extension using one
looks like a verb is missing
| switch (igniteInstanceName) { | ||
| case NODE_0: | ||
| igniteConfiguration.setConsistentId(NODE_0); | ||
| break; | ||
|
|
||
| case NODE_1: | ||
| igniteConfiguration.setConsistentId(NODE_1); | ||
| break; | ||
|
|
||
| case NODE_2: | ||
| igniteConfiguration.setConsistentId(NODE_2); | ||
| break; | ||
|
|
||
| default: throw new IllegalArgumentException("Unknown node: " + igniteInstanceName); | ||
| } |
There was a problem hiding this comment.
Looks like switch is overhead here and we can simplify to smth like:
return super.getConfiguration(igniteInstanceName)
.setConsistentId(igniteInstanceName)
.setClusterStateOnStart(INACTIVE)
.setGridLogger(listeningLog);2 participants
No description provided.