IGNITE-16717 Implement node validation #795
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sashapolo
force-pushed
the
ignite-16717
branch
11 times, most recently
from
6f71818 to
a286f6d
Compare
sashapolo
force-pushed
the
ignite-16717
branch
2 times, most recently
from
c17e14a to
81715e2
Compare
rpuch
reviewed
May 19, 2022
...rationTest/java/org/apache/ignite/internal/cluster/management/raft/ItCmgRaftServiceTest.java
Show resolved
Hide resolved
...rationTest/java/org/apache/ignite/internal/cluster/management/raft/ItCmgRaftServiceTest.java
Outdated
Show resolved
Hide resolved
...rationTest/java/org/apache/ignite/internal/cluster/management/raft/ItCmgRaftServiceTest.java
Outdated
Show resolved
Hide resolved
...anagement/src/main/java/org/apache/ignite/internal/cluster/management/LocalStateStorage.java
Outdated
Show resolved
Hide resolved
...t/src/main/java/org/apache/ignite/internal/cluster/management/raft/CmgRaftGroupListener.java
Outdated
Show resolved
Hide resolved
| private final byte major; | ||
|
|
||
| /** Minor version number. */ | ||
| private final byte minor; |
Contributor
There was a problem hiding this comment.
Why are we restricting version components with byte? Is it imposed by the semver spec? If not, let's make it int just in case.
Contributor
Author
There was a problem hiding this comment.
I copied this part from Ignite 2. I'd prefer to leave it as a byte for now
modules/core/src/main/java/org/apache/ignite/internal/properties/IgniteProductVersion.java
Outdated
Show resolved
Hide resolved
modules/core/src/main/java/org/apache/ignite/internal/properties/IgniteProductVersion.java
Outdated
Show resolved
Hide resolved
| * Extracts the local state (if any) and starts the CMG. | ||
| * | ||
| * @return Future that resolves into the CMG Raft service or {@code null} if the local state is empty. | ||
| * @return Future, that resolves into the CMG Raft service, or {@code null} if the local state is empty. |
Contributor
There was a problem hiding this comment.
Would they really put that comma after 'Future' in English?
Contributor
Author
There was a problem hiding this comment.
I've put these commas to emphasize the fact that the future itself can be null and it will not resolve to a null value
| ValidationError validationError = NodeValidator.validateNode(state, validationInfo); | ||
|
|
||
| if (validationError != null) { | ||
| throw new IgniteInternalException(validationError.rejectReason()); |
Contributor
There was a problem hiding this comment.
Should a specific exception class be introduced for self-validation failures?
Contributor
Author
There was a problem hiding this comment.
I don't know, why?
Contributor
Author
There was a problem hiding this comment.
Actually this is also no longer valid, I've reworked self-validation to work in the same way as the other validation
Contributor
There was a problem hiding this comment.
It could make diagnostics a bit easier by adding some context: not just 'why it failed', but also 'what failed'.
sanpwc
requested changes
May 20, 2022
modules/api/src/main/java/org/apache/ignite/IgnitionManager.java
Outdated
Show resolved
Hide resolved
...integrationTest/java/org/apache/ignite/internal/cluster/management/ItClusterManagerTest.java
Show resolved
Hide resolved
...rationTest/java/org/apache/ignite/internal/cluster/management/raft/ItCmgRaftServiceTest.java
Outdated
Show resolved
Hide resolved
| Collection<String> cmgNodeNames, | ||
| String clusterName | ||
| ) { | ||
| if (metaStorageNodeNames.isEmpty()) { |
Contributor
There was a problem hiding this comment.
Please check my comment about params validation in IgnitionManager.
...src/main/java/org/apache/ignite/internal/cluster/management/raft/ValidationTokenManager.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/ignite/internal/cluster/management/raft/ValidationTokenManager.java
Outdated
Show resolved
Hide resolved
...management/src/main/java/org/apache/ignite/internal/cluster/management/rest/InitCommand.java
Show resolved
Hide resolved
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java
Show resolved
Hide resolved
...ent/src/main/java/org/apache/ignite/internal/cluster/management/raft/RaftStorageManager.java
Show resolved
Hide resolved
sashapolo
force-pushed
the
ignite-16717
branch
3 times, most recently
from
3d40358 to
993ee6d
Compare
sashapolo
force-pushed
the
ignite-16717
branch
2 times, most recently
from
98e23c5 to
6b996f8
Compare
rpuch
approved these changes
May 23, 2022
sanpwc
approved these changes
May 24, 2022
isapego
pushed a commit
to isapego/ignite-3
that referenced
this pull request
Dec 26, 2024
GG-39353 Handle VacuumTxStatesCommand --------- Co-authored-by: Kirill Sizov <sizov.kirill.y@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://issues.apache.org/jira/browse/IGNITE-16717