IGNITE-13190

[alievmirza]

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

• How to Contribute
• Coding abbreviation rules
• Coding Guidelines
• Apache Ignite Teamcity Bot

If you need any help, please email [email protected] or ask anу advice on http://asf.slack.com #ignite channel.

[Mmuzaf]

What would happen with this exception? You will fail the node or it will be ignored? Will it be logged?

[Mmuzaf]

You throw IgniteCheckedException inside renameTempPartitionFile than catch it here and rethrow IgniteException.
Is it better to throw IgniteException right from renameTempPartitionFile method? Why we need checked exception here?

[ibessonov]

Checked exceptions are a standard for Java, that's why I use them when I can.
Exception will not be ignored, node will fail later because it couldn't complete checkpoint.

[Mmuzaf]

I don't think we need it here, let's throw directly IgniteException? This will simplify the usage.

[ibessonov]

Ok, I'm cool with it, not a big deal. It'll make other places more complicated though

[Mmuzaf]

Please, remove it.

[Mmuzaf]

Let's move everything here from PageStoreMap here? The DefragmentationPageReadWriteManager will also implement the PageStoreCollection.

[ibessonov]

No, DefragmentationPageReadWriteManager is instantiated later then we need page stores map. Why would I put this code inside of the manager? Reason #2 - we have two maps, not one.

[ibessonov]

Please, remove it.

Please show me how do I use TimeBag for the same scenario. I'm sure that I can't do that.

It will be simpler and better if you'll reuse try-finally approach for all these cases.

Ok, I'm removing all this code.

[Mmuzaf]

cpLock and log should be part of API. You can obtain it from the context in implementation.

[ibessonov]

I agree about log, it'll be removed. On the other hand - this is not the same checkpoint lock that you think, it's lock from specific defragmentation checkpointer. I'm not removing it.

[Mmuzaf]

Yes. Is it better calling the whole defragment method for indexing under CachePartitionDefragmentationManager and remove cpLock from the method parameters?

        defragmentationCheckpoint.checkpointTimeoutLock().checkpointReadLock();
        
        try {
            idx.defragment(
                grpCtx,
                newCtx,
                (PageMemoryEx)partDataRegion.pageMemory(),
                mappingByPartition
            );
        }
        finally {
            defragmentationCheckpoint.checkpointTimeoutLock().checkpointReadUnlock();
        }

[ibessonov]

No, we don't want it to be under a single lock, it can overflow page memory. We have to release checkpoint lock and acquire it again sometimes.