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-19950 Cache dumps implemented #10950

Merged
merged 19 commits into from
Oct 9, 2023
Merged

IGNITE-19950 Cache dumps implemented #10950

merged 19 commits into from
Oct 9, 2023

Conversation

nizhikov
Copy link
Contributor

Thank you for submitting the pull request to the Apache Ignite.

In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:

The Contribution Checklist

  • 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 pull request body describes changes that have been made.
    The description explains WHAT and WHY was made instead of 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.
  • A reviewer has been mentioned through the JIRA comments
    (see the Maintainers list)
  • The pull request has been checked by the Teamcity Bot and
    the green visa attached to the JIRA ticket (see TC.Bot: Check PR)

Notes

If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.

@nizhikov nizhikov changed the title IGNITE-19950 Cache dumps implemented (#10936) [WIP] IGNITE-19950 Cache dumps implemented (#10936) Sep 22, 2023
@nizhikov nizhikov changed the title [WIP] IGNITE-19950 Cache dumps implemented (#10936) IGNITE-19950 Cache dumps implemented (#10936) Oct 6, 2023
@nizhikov nizhikov changed the title IGNITE-19950 Cache dumps implemented (#10936) IGNITE-19950 Cache dumps implemented Oct 6, 2023
import static org.apache.ignite.internal.pagemem.PageIdAllocator.INDEX_PARTITION;

/**
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's collapse it into one line or provide a meaningful Javadoc.

* Regular count of partitions is {@link RendezvousAffinityFunction#DFLT_PARTITION_COUNT}
* and thread is {@link IgniteConfiguration#DFLT_PUBLIC_THREAD_CNT} whic is significantly less.
*/
private final ConcurrentMap<Long, ByteBuffer> thLocBufs = new ConcurrentHashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I still believe that it's better to use some kind of pooled memory allocator 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.

For what reason? Can you, please, explain, what is wrong with current approach and how memory allocator will make things better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

* Regular count of partitions is {@link RendezvousAffinityFunction#DFLT_PARTITION_COUNT}
* and thread is {@link IgniteConfiguration#DFLT_PUBLIC_THREAD_CNT} whic is significantly less.
*/
private final ConcurrentMap<Long, ByteBuffer> thLocBufs = new ConcurrentHashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I still believe that it's better to use some kind of pooled memory allocator 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.

For what reason? Can you, please, explain, what is wrong with current approach and how memory allocator will make things better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

* @param raw If {@code true} then keep read entries in form of {@link KeyCacheObject} and {@link CacheObject}.
* @param log Logger.
*/
public Dump(File dumpDir, @Nullable String consistentId, boolean keepBinary, boolean raw, IgniteLogger log) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The Dump class API doesn't seem consistent to me. #partitions(), #metadata(), #iterator() etc methods require a node argument, although we have a dedicated constructor to create a node-specific dump.

Copy link
Contributor Author

@nizhikov nizhikov Oct 9, 2023

Choose a reason for hiding this comment

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

There are two scenario when we analyzing dump content:

  1. User exploring dump content with DumpReader. So Dump provide a way to consume all data saved in dump.

  2. Ignite checks dump consistency during creation. In this case we don't want to know or even read data belong to other nodes, because, other node can be in the middle of dump creation. That's why we want to have the way to filter out all node except one in constructor.

Note, that Dump is internal class, so it's not an API.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 9, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug E 6 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 74 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@nizhikov nizhikov merged commit 0419fff into master Oct 9, 2023
8 of 9 checks passed
J-Bakuli pushed a commit to J-Bakuli/ignite that referenced this pull request Nov 10, 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
Development

Successfully merging this pull request may close these issues.

2 participants