Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IGNITE-12701 : Disallow silent deactivation in CLI and REST. #7471

Merged
merged 85 commits into from
Mar 11, 2020

Conversation

Vladsz83
Copy link
Contributor

@Vladsz83 Vladsz83 commented Feb 25, 2020

Changes in public behavior:

  1. Involved optional flag 'force' for control.sh (bat) with --deactivate and --set-sate:
    control.sh --deactivate --force
    control.sh --set-state INACTIVE --force

  2. Involved optional parameter 'force=true|false' for REST cluster state requests:
    http://127.0.0.1:8092/ignite?cmd=deactivate&force=true
    http://127.0.0.1:8092/ignite?cmd=inactive&force=true
    http://127.0.0.1:8092/ignite?cmd=setstate&state=INACTIVE&force=true

Major code changes:

  1. Added GridClientClusterStateRequestV2.
  2. Deprecated GridClientClusterStateRequest.
  3. Added flag ‘force’ for REST in GridRestClusterStateRequest
  4. Added flag ‘force’ CLI in DeactivateCommand, ClusterStateChangeCommand.
  5. Added IgniteFeatures# FORCED_CHANGE_OF_CLUSTER_STATE.
  6. Added checking of deactivation safety: GridClusterStateProcessor# isDeactivationSafe()
  7. Involved o deactivation safety checking in GridClusterStateProcessing and GridClientClusterStateImpl.

Vladsz83 and others added 30 commits February 3, 2020 19:20
new IgniteCluster#state(ClusterState, boolean force)
@@ -1617,6 +1643,7 @@ public ExchangeActions autoAdjustExchangeActions(ExchangeActions exchActs) {
ctx.localNodeId(),
null,
ClusterState.active(clusterState.state()) ? clusterState.state() : ACTIVE,
true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear why we force deactivation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nizhikov nizhikov merged commit 4921fcf into apache:master Mar 11, 2020
@Vladsz83 Vladsz83 deleted the IGNITE-12701-3 branch March 11, 2020 17:00
@sk0x50
Copy link
Contributor

sk0x50 commented Mar 15, 2020

Hello @nizhikov ,

Please don't get me wrong, but...
What is the reason to include all intermediate commit messages into the final one? 4921fcf
Moreover, it contains the following tickets: IGNITE-12597, IGNITE-12646, and IGNITE-12703 that not related to the issue at all. It looks like it was done by accident.

@Vladsz83
Copy link
Contributor Author

Moreover, it contains the following tickets: IGNITE-12597, IGNITE-12646, and IGNITE-12703 that not related to the issue at all. It looks like it was done by accident.

Hi, @sk0x50

This merge doesn't contain mentioned issues IGNITE-12597, IGNITE-12646, and IGNITE-12703 at all. That was just wrong comments in my branch.

@sk0x50
Copy link
Contributor

sk0x50 commented Mar 15, 2020

Hello @Vladsz83 ,

This merge doesn't contain mentioned issues IGNITE-12597, IGNITE-12646, and IGNITE-12703 at all. That was just wrong comments in my branch.

Yes, I know that, and you are right.
Perhaps, my message was not quite correct. I just wanted to say that it contains incorrect commit messages that can lead to a misunderstanding. That's all.

Thanks,
Slava.

@nizhikov
Copy link
Contributor

Hello @sk0x50

First of all, this is my fault, but the reason for the fault is GitHub bug.
I wrote a good commit message before the merge.

But after pushing the "merge" button I've got "We can't merge your PR" error message and "Try again" button. After pushing "Try again" we got what we got - merge with the default commit message.

Sorry, for that.

@sk0x50
Copy link
Contributor

sk0x50 commented Mar 16, 2020

Hello @nizhikov,

Ok, got it! Thank you for the clarification.

Thanks,
S.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants