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-18554 Remove MetaStorage learners on topology events #1542

Merged
merged 8 commits into from
Feb 14, 2023

Conversation

sashapolo
Copy link
Contributor

@sashapolo sashapolo changed the title IGNITE-18554 Remove MetaStorage learners on topology events @sashapolo IGNITE-18554 Remove MetaStorage learners on topology events Jan 17, 2023
@sashapolo sashapolo marked this pull request as ready for review January 22, 2023 16:50
@sashapolo sashapolo force-pushed the ignite-18554 branch 2 times, most recently from 0b03ce9 to fdbb0f1 Compare January 27, 2023 10:18
@sashapolo sashapolo closed this Feb 8, 2023
@sashapolo sashapolo deleted the ignite-18554 branch February 8, 2023 15:13
@sashapolo sashapolo restored the ignite-18554 branch February 8, 2023 15:13
@sashapolo sashapolo reopened this Feb 8, 2023
@sashapolo sashapolo force-pushed the ignite-18554 branch 5 times, most recently from bfa674c to ffd9bbd Compare February 10, 2023 14:06
logicalTopologyService.addEventListener(new LogicalTopologyEventListener() {
@Override
public void onNodeValidated(ClusterNode validatedNode) {
executeIfLeader((service, term) -> addLearner(service.raftGroupService(), validatedNode));
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. We do call onNodeValidated, ..., onNodeLeft are called within raft thread, is that correct?
  2. If true
    2.a You didn't introduce any synchronous long running logic, e.g. network communication as a reaction to such events, did you? serializationFuture?
    2.b You didn't introduce any logic that may prevent CMG state-machine idempotentancy, did you? Meaning, that if we'll have an exception within onSmt on Node A and won't have such an exception on Node B, CMG listener won't be effected, is that true?
  3. Who is responsible for exception handling for serializationFuture? E.g. logging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do call onNodeValidated, ..., onNodeLeft are called within raft thread, is that correct?

yes

You didn't introduce any synchronous long running logic, e.g. network communication as a reaction to such events, did you? serializationFuture?

How is it possible to introduce synchronous network calls in our code? All such stuff is done asynchronously. That's exactly why I need the serializationFuture - to chain a bunch of asynchronous calls to preserve Raft linearization guarantees. But I never wait on on this future.

You didn't introduce any logic that may prevent CMG state-machine idempotentancy, did you? Meaning, that if we'll have an exception within onSmt on Node A and won't have such an exception on Node B, CMG listener won't be effected, is that true?

Since all operations are asynchronous, I expect all exception to simply fail on the related thread and be possibly logged somewhere.

Who is responsible for exception handling for serializationFuture? E.g. logging.

Logging is done inside the action that is passed to executeIfLeaderImpl. For example, see the updateConfigUnderLock method, it should do all the logging.

@sanpwc sanpwc merged commit dc56fe7 into apache:main Feb 14, 2023
@sanpwc sanpwc deleted the ignite-18554 branch February 14, 2023 12:43
lowka pushed a commit to gridgain/apache-ignite-3 that referenced this pull request Mar 18, 2023
lowka pushed a commit to gridgain/apache-ignite-3 that referenced this pull request Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants