Skip to content

IGNITE-18456 Explain treading model in corresponding README.md file f…#1493

Merged
vldpyatkov merged 7 commits into
apache:mainfrom
gridgain:ignite-18456
Jan 12, 2023
Merged

IGNITE-18456 Explain treading model in corresponding README.md file f…#1493
vldpyatkov merged 7 commits into
apache:mainfrom
gridgain:ignite-18456

Conversation

@vldpyatkov

@vldpyatkov vldpyatkov commented Jan 4, 2023

Copy link
Copy Markdown
Contributor

Comment thread modules/metastorage/README.md Outdated
Comment thread modules/metastorage/README.md Outdated

The module for store and access to metadata.

The storage is distributed to prevent losing data and based of RAFT. In typical case, the group consists of only several nodes of the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, that's not only to prevent data loss but also for fault tolerance, etc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Failure tolerance has the same stance in case of one node leaves the cluster and data should be still available. In my mind, the prevention of losing data is a general characteristic for all requirements that an any distributed system has.

Comment thread modules/metastorage/README.md Outdated

The module for store and access to metadata.

The storage is distributed to prevent losing data and based of RAFT. In typical case, the group consists of only several nodes of the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In typical case, the group consists of only several nodes of the...

Could you please rephrase it? Currently it seems to be a little bit ambiguous. Let's say that meta storage group has it's nodes on top of every cluster node, however most of such participants are learners (here we should explain what raft learner is) and only few (3, 5) of them are voting ones. Basically it's what you've already told above but with greater details.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I reformulated this part.

Comment thread modules/metastorage/README.md Outdated
Comment thread modules/metastorage/README.md Outdated

### Interface methods

Various operations (*get()*, *getAll()*, *invoke()*) in Metastorage manager return futures. Those futures are completed when the matched

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As we've discussed let's add few more details here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added link to raft module.

this.metaStorageSvcFut = cmgMgr.metaStorageNodes()
// use default executor to avoid blocking CMG manager threads
.thenComposeAsync(metaStorageNodes -> {
.thenCompose(metaStorageNodes -> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is that about

The component is used common ForkJoinPool on start

?
There's a comment

// use default executor to avoid blocking CMG manager threads

So, do you really think that using thenCompose instead of thenComposeAsync won't block CMG manager threads?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am sure, because the code does not execute in CMG thread after all components began start in the common thread pool.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The comment about CMG thread are removed from here.

Both of the executors have no a node name in their names. The names are required to identify threads belonged to different Ignite nodes on
the shared java machine (TODO: IGNITE-18504 Add nodes name in prefix to Metastorage executors).

### Interface methods

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Initially, there were asyncs on cursors, we do expect this to be reworked with subscribers, correct? If true this subscribers will probably add some (one?) new threads. Could you please create a follow-up ticket in order to update ms thread-model after Alexander will finish his ms-cursors refactoring?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We even have no an JIRA ticket to redesign cursor API to reactive model for Metastorage. But I notified @sashapolo about the modification can be required after the API will have changed.

Comment thread modules/metastorage/README.md Outdated
Comment thread modules/metastorage/README.md Outdated
Comment thread modules/metastorage/README.md
Comment thread modules/metastorage/README.md
Comment thread modules/metastorage/README.md
Comment thread modules/metastorage/README.md
Comment thread modules/metastorage/README.md Outdated
Comment thread modules/metastorage/README.md
Comment thread modules/metastorage/README.md
threads, this result appears in the RAFT client executor with prefix <NODE_NAME>%Raft-Group-Client. See RAFT module for more information
about its threading model.

Although some methods return futures, they are often run synchronously. Futures are dependent on asynchronous Metastorage initialization,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn'we specify these methods? "some methods" sounds too uncertain

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I do not shut what exactly methods can appear in Metastorage manager, except method that are using to access data (but they are, for example, method of stopping watch). Rather, I mean this about all method that does not connected with data access, but return a future.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't know exactly because this moment the Metastorae API is changing.

Comment thread modules/metastorage/README.md Outdated
Comment thread modules/metastorage/README.md Outdated
Comment thread modules/metastorage/README.md Outdated
@vldpyatkov
vldpyatkov merged commit d4de4f6 into apache:main Jan 12, 2023
@vldpyatkov
vldpyatkov deleted the ignite-18456 branch January 12, 2023 12:11
maxzhuravkov pushed a commit to gridgain/apache-ignite-3 that referenced this pull request Mar 18, 2023
maxzhuravkov pushed a commit to gridgain/apache-ignite-3 that referenced this pull request Apr 19, 2023
isapego pushed a commit to isapego/ignite-3 that referenced this pull request Dec 26, 2024
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.

4 participants