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.

Sign up for GitHub

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

Jump to bottom

IGNITE-22241 Make ClusterManagementGroupManager extensible #3756

Merged
merged 7 commits into from
May 24, 2024
Merged

IGNITE-22241 Make ClusterManagementGroupManager extensible #3756

merged 7 commits into from
May 24, 2024

Conversation

tmgodinho
Copy link
Contributor

@tmgodinho tmgodinho commented May 14, 2024 •
edited

Thank you for submitting the pull request.

To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:

The Review Checklist

  • Formal criteria: TC status, codestyle, mandatory documentation. Also make sure to complete the following:
    - 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 description of the JIRA ticket explains WHAT was made, WHY and 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.
  • Design: new code conforms with the design principles of the components it is added to.
  • Patch quality: patch cannot be split into smaller pieces, its size must be reasonable.
  • Code quality: code is clean and readable, necessary developer documentation is added if needed.
  • Tests code quality: test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.

Notes

ignite-22241: Made ClusterManagementGroupManager extensible.
c3d4cd6 
** Introduced ClusterManagerGroupEvents event to ClusterManagementGroup.
@tmgodinho tmgodinho marked this pull request as ready for review May 15, 2024 06:06
@tmgodinho
Copy link
Contributor Author

Hey @rpuch and @sashapolo, would you mind having a look at these changes?
Do you think it's necessary to introduce some tests to check if the events are fired? Didn't seem very error-prone to me.

sashapolo
sashapolo reviewed May 15, 2024
View reviewed changes
...c/main/java/org/apache/ignite/internal/cluster/management/ClusterManagementGroupManager.java Outdated
@@ -434,6 +441,12 @@ private void handleCancelInit(CancelInitMessage msg) {
* Completely destroys the local CMG Raft service.
*/
private void destroyCmg() {
try {
fireEvent(ClusterManagerGroupEvent.BEFORE_RAFT_GROUP_DESTROY, EmptyEventParams.INSTANCE).join();
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 better to use get with a timeout. Also, this method is called inside a network thread, we should probably dispatch it onto a thread pool or make it asynchronous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a few doubts about this suggestion.
Currently, the destroyCmg method runs and blocks the same thread as the message handler. I believe this is what you called network thread.
When you say to put it on a thread pool or make it asynchronous are you suggesting that we don't wait for the destroy to finish, i.e. let it run in the background without calling get or join since it would still block the network thread??

Copy link
Contributor

Choose a reason for hiding this comment

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

How about not using get/join at all? Just collect the listeners' futures to one using CompletableFuture#allOf() and than on that aggregated future make .thenRun() doing the rest of the method; destroyCmg() would return a CompletableFuture as well; the code that calls it and needs to execute some logic strictly after the CMG has been destroyed can just chain on the future it returns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, there's no code after destoryCmg just the error handling message and the busyLock unlock.

Copy link
Contributor

Choose a reason for hiding this comment

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

In handleClusterState(), there is a call to destroyCmg(), and there is a initCmgRaftService(state) call that probably relies on the CMG being destroyed at the moment when it's made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I saw that afterwards. Missed that one on the first iteration.

...main/java/org/apache/ignite/internal/cluster/management/events/ClusterManagerGroupEvent.java Outdated Show resolved Hide resolved
...main/java/org/apache/ignite/internal/cluster/management/events/ClusterManagerGroupEvent.java Outdated Show resolved Hide resolved
...main/java/org/apache/ignite/internal/cluster/management/events/ClusterManagerGroupEvent.java Outdated Show resolved Hide resolved
...ent/src/main/java/org/apache/ignite/internal/cluster/management/events/EmptyEventParams.java Outdated Show resolved Hide resolved
...ent/src/main/java/org/apache/ignite/internal/cluster/management/events/EmptyEventParams.java Outdated Show resolved Hide resolved
...rg/apache/ignite/internal/cluster/management/events/BeforeStartRaftGroupEventParameters.java Outdated Show resolved Hide resolved
...rg/apache/ignite/internal/cluster/management/events/BeforeStartRaftGroupEventParameters.java Outdated Show resolved Hide resolved
ignite-22241: Applied code-reviews
cf1fedc 
** Renamed EmptyEventParameters
** Renamed BEFORE_RAFT_GROUP_DESTROY to BEFORE_DESTROY_RAFT_GROUP
** Replaced HashSet::new in favour ot Set::copyOf
@tmgodinho tmgodinho changed the title ignite-22241: Made ClusterManagementGroupManager extensible IGNITE-22241 Made ClusterManagementGroupManager extensible May 15, 2024
Tiago Marques Godinho added 3 commits May 16, 2024 16:50
IGNITE-22241 Applies code-reviews
82d51cf 
** Removed helper function to convert Consumer to Function
** Replaced handle with whenComplete in destroyCmgWithEvents future
** Start calling desctroyCmg() in the scheduledThreadPool
IGNITE-22241 Reverts changes to CmgMessageHandlerFactory
3858c52 
** They are no longer needed.

#fixes (73779cb) Start handling ClusterManagementGroupManager in an async way
IGNITE-22241 Adds FailureProcessor to CMGManager
7ce0b12 
** Most of the changes are updating dependencies in tests. It would be great to have DI or fixtures for not replicating so much and be more consistent across tests.
@tmgodinho tmgodinho requested review from sashapolo and rpuch May 16, 2024 15:51
@rpuch rpuch changed the title IGNITE-22241 Made ClusterManagementGroupManager extensible IGNITE-22241 Make ClusterManagementGroupManager extensible May 22, 2024
@rpuch rpuch merged commit 6fcadcb into apache:main May 24, 2024
1 check passed
@rpuch rpuch deleted the ignite-22241 branch May 24, 2024 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sashapolo sashapolo sashapolo approved these changes

@rpuch rpuch rpuch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@tmgodinho @sashapolo @rpuch