You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2020/06/11 09:55:40 UTC
[GitHub] [ignite] antonovsergey93 opened a new pull request #7924: IGNITE-13144 minor usability and code improvements in a cluster read-only mode.
antonovsergey93 opened a new pull request #7924:
URL: https://github.com/apache/ignite/pull/7924
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
----------------------------------------------------------------
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
[GitHub] [ignite] antonovsergey93 commented on a change in pull request #7924: IGNITE-13144: minor usability and code improvements in a cluster read-only mode.
Posted by GitBox <gi...@apache.org>.
antonovsergey93 commented on a change in pull request #7924:
URL: https://github.com/apache/ignite/pull/7924#discussion_r442959379
##########
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:
Agree
----------------------------------------------------------------
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
[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.
Posted by 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
[GitHub] [ignite] antonovsergey93 commented on a change in pull request #7924: IGNITE-13144: minor usability and code improvements in a cluster read-only mode.
Posted by GitBox <gi...@apache.org>.
antonovsergey93 commented on a change in pull request #7924:
URL: https://github.com/apache/ignite/pull/7924#discussion_r442959775
##########
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:
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:
users@infra.apache.org
[GitHub] [ignite] antonovsergey93 commented on a change in pull request #7924: IGNITE-13144: minor usability and code improvements in a cluster read-only mode.
Posted by GitBox <gi...@apache.org>.
antonovsergey93 commented on a change in pull request #7924:
URL: https://github.com/apache/ignite/pull/7924#discussion_r442958036
##########
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:
I've added force checkpoint by mistake. I thought that it could be helpful for the cluster backup in read-only mode. But, I forgot about rebalance and other inner activities. So, force checkpoint not needed there.
----------------------------------------------------------------
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
[GitHub] [ignite] antonovsergey93 commented on a change in pull request #7924: IGNITE-13144: minor usability and code improvements in a cluster read-only mode.
Posted by GitBox <gi...@apache.org>.
antonovsergey93 commented on a change in pull request #7924:
URL: https://github.com/apache/ignite/pull/7924#discussion_r442958868
##########
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:
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:
users@infra.apache.org
[GitHub] [ignite] asfgit closed pull request #7924: IGNITE-13144: minor usability and code improvements in a cluster read-only mode.
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #7924:
URL: https://github.com/apache/ignite/pull/7924
----------------------------------------------------------------
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