IGNITE-18550 Add idle_verify check of incremental snapshots #10617

timoninmaxim · 2023-03-29T13:12:52Z

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 · 2023-04-05T17:35:47Z

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java

+                    }
+                });
+
+                // All active transactions that didn't log COMMITTED or ROLL_BACK records are considered committed.


Can transactions without WAL record be rolled back during node recovery?
In case node fails after incremental snapshot created but before TX_RECORD loged

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java

nizhikov · 2023-04-05T20:37:18Z

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java

+    /** Holder for parition hashes. */
+    private static class PartitionHashHolder {
+        /** */
+        private int hash;


Looks like these fields should be long

I made it as int, because PartitionHashRecordV2 stores hashes as int too: partHash, partVerHash.

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java

nizhikov · 2023-04-07T09:34:01Z

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java

+                    }
+                };
+
+                short locShortId = blt.consistentIdMapping().get(consId);


...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java

nizhikov · 2023-04-07T09:34:32Z

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java

+                Map<PartitionKeyV2, PartitionHashHolder> partMap = new HashMap<>();
+                List<Exception> exceptions = new ArrayList<>();
+
+                BiConsumer<GridCacheVersion, Set<Short>> calcTransactionHash = (xid, partNodes) -> {


calcTxHash?

nizhikov · 2023-04-07T09:34:53Z

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java

+                List<Exception> exceptions = new ArrayList<>();
+
+                BiConsumer<GridCacheVersion, Set<Short>> calcTransactionHash = (xid, partNodes) -> {
+                    for (short shortId: partNodes) {


...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java

nizhikov · 2023-04-07T09:43:34Z

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java

+                Map<GridCacheVersion, Set<Short>> txPrimPartNodes = new HashMap<>();
+                Map<Short, PartitionHashHolder> nodesTxHash = new HashMap<>();
+
+                Set<GridCacheVersion> partialCommittedTxs = new HashSet<>();


partiallyCommittedTxs here and in all other places?

nizhikov · 2023-04-07T09:47:34Z

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java

+                Map<Short, PartitionHashHolder> nodesTxHash = new HashMap<>();
+
+                Set<GridCacheVersion> partialCommittedTxs = new HashSet<>();
+                Map<PartitionKeyV2, PartitionHashHolder> partMap = new HashMap<>();


Please, add comment that hashes in this map calculated based on WAL records, not part-X.bin data so they will differs from idle_verify produced hashes.

nizhikov · 2023-04-07T09:49:51Z

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java

+                        hash.increment(valHash, verHash);
+                    }
+                    catch (IgniteCheckedException ex) {
+                        exceptions.add(ex);


Do we really want to continue task in case of any error?
Looks like we can just skip all further actions like:

proc.process(dataEntry -> { if (dataEntry.op() == GridCacheOperation.READ || !exceptions.isEmpty()) return; ... }, txRec -> { if (!exceptions.isEmpty()) return; if (log.isDebugEnabled()) log.debug("Checking tx record [txRec=" + txRec + ']');

nizhikov · 2023-04-07T10:10:39Z

...nternal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTaskResult.java

+    /** Transaction hashes collection. */
+    private Map<Object, TransactionsHashRecord> txHashRes;
+
+    /** Partition hashes collection. It is calculated on data entries included into only incremental part of snapshot. */


Suggested change

/** Partition hashes collection. It is calculated on data entries included into only incremental part of snapshot. */

/**

* Partition hashes collection.

* Hash of data entries({@link DataEntry}) from WAL segments included in incremental snapshot.

*/

nizhikov · 2023-04-07T10:19:46Z

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java

+                            null)
+                    ));
+
+                return new IncrementalSnapshotVerificationTaskResult(


Let's locally log verification results here on info level.
Including number of dataEntry (proc.applied) and transactions

sonarqubecloud · 2023-04-08T19:39:53Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
58 Code Smells

0.0% Coverage
1.0% Duplication

timoninmaxim added 8 commits March 29, 2023 16:12

IGNITE-18550 Add idle_verify check of incremental snapshots

64cc09f

Fix checkstyle

a883ba9

Fix tests

3c8b009

Merge branch 'master' into IGNITE-18550__verify_inc_snp

416e7df

Add partition data check

3cf2925

Merge branch 'master' into IGNITE-18550__verify_inc_snp

256b0d0

Fix checkstyle

5e1b4a6

Fix minors

2686f16

nizhikov reviewed Apr 5, 2023

View reviewed changes

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java Outdated Show resolved Hide resolved

nizhikov reviewed Apr 5, 2023

View reviewed changes

nizhikov reviewed Apr 6, 2023

View reviewed changes

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java Show resolved Hide resolved

nizhikov reviewed Apr 6, 2023

View reviewed changes

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java Show resolved Hide resolved

nizhikov reviewed Apr 6, 2023

View reviewed changes

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java Show resolved Hide resolved

nizhikov reviewed Apr 6, 2023

View reviewed changes

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java Outdated Show resolved Hide resolved

nizhikov reviewed Apr 6, 2023

View reviewed changes

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java Outdated Show resolved Hide resolved

nizhikov reviewed Apr 6, 2023

View reviewed changes

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java Outdated Show resolved Hide resolved

Fix review

5767917

nizhikov reviewed Apr 7, 2023

View reviewed changes

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java Show resolved Hide resolved

nizhikov reviewed Apr 7, 2023

View reviewed changes

...nite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotVerificationTask.java Outdated Show resolved Hide resolved

nizhikov reviewed Apr 7, 2023

View reviewed changes

Fix reviw

91ccb4e

timoninmaxim force-pushed the IGNITE-18550__verify_inc_snp branch from addbcee to 91ccb4e Compare April 7, 2023 13:22

Fix reviw

d339552

nizhikov approved these changes Apr 7, 2023

View reviewed changes

timoninmaxim added 3 commits April 7, 2023 18:51

Fix namings

6bb5f23

Fix empty exceptions

bd6652a

Add docs, add test for removed data

4ddc5dc

timoninmaxim merged commit d0d3ead into apache:master Apr 8, 2023

timoninmaxim deleted the IGNITE-18550__verify_inc_snp branch April 8, 2023 19:53

IGNITE-18550 Add idle_verify check of incremental snapshots #10617

IGNITE-18550 Add idle_verify check of incremental snapshots #10617

Uh oh!

Conversation

timoninmaxim commented Mar 29, 2023

The Contribution Checklist

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Apr 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants