Skip to content

HDDS-8477. Unit test for Snapdiff using tombstone entries #4678

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.

Sign up for GitHub

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

Jump to bottom
Merged
prashantpogde merged 27 commits into from
Jun 9, 2023
Merged

HDDS-8477. Unit test for Snapdiff using tombstone entries #4678

prashantpogde merged 27 commits into from
Jun 9, 2023

Conversation

@swamirishi
Copy link
Contributor

@swamirishi swamirishi commented May 8, 2023 •
edited
Loading

What changes were proposed in this pull request?

Unit test for Snapdiff using tombstone entries, changes related to [HDDS-8137]
SSTDumpToolIterator was failing in the case key or value contain '\0' character, since prinf in c++ limits the string formatting till the first occurance null character. This PR also contains changes to send raw bytes of key, value over the pipe from the native sst file reader & corresponding changes to deserialize the raw bytes from the stream.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-8477

How was this patch tested?

Adding unit tests

@swamirishi swamirishi changed the title Hdds 8477 HDDS-8477. Unit test for Snapdiff using tombstone entries May 8, 2023
@swamirishi swamirishi marked this pull request as draft May 8, 2023 17:09
@aswinshakil aswinshakil added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label May 9, 2023
@swamirishi swamirishi marked this pull request as ready for review May 10, 2023 18:06
smengcl
smengcl reviewed May 10, 2023
View reviewed changes
smengcl
smengcl reviewed May 10, 2023
View reviewed changes
smengcl
smengcl reviewed May 10, 2023
View reviewed changes
smengcl
smengcl reviewed May 10, 2023
View reviewed changes
@swamirishi swamirishi mentioned this pull request May 17, 2023
Merged
hemantk-12
hemantk-12 reviewed May 23, 2023
View reviewed changes
Copy link
Contributor

@hemantk-12 hemantk-12 left a comment

Choose a reason for hiding this comment

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

Thanks for the patch @swamirishi. Left some comments.

swamirishi added 4 commits May 23, 2023 17:51
@swamirishi
HDDS-8477: Address review comments
a51fb2b
@swamirishi
HDDS-8477: Fix checkstyle
607c185
@swamirishi
Merge remote-tracking branch 'apache/master' into HEAD
0611a6e 
# Conflicts:
#	hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshot.java
#	hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java
@swamirishi
HDDS-8477: Fix checkstyle
964d5a6
hemantk-12
hemantk-12 reviewed May 24, 2023
View reviewed changes
@swamirishi swamirishi mentioned this pull request Jun 2, 2023
Merged
@swamirishi
Copy link
Contributor Author

swamirishi commented Jun 2, 2023

@DaveTeng0 the snapshot delete robot test is failing. I have fixed it as part of thir PR pls take a look if the changes look fine.

adoroszlai
adoroszlai requested changes Jun 2, 2023
View reviewed changes
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @swamirishi for working on this. (I have only reviewed the CI-related part.)

hemantk-12
hemantk-12 reviewed Jun 6, 2023
View reviewed changes
This was referenced Jun 8, 2023
Merged
Merged
hemantk-12
hemantk-12 reviewed Jun 8, 2023
View reviewed changes
...one/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java Outdated

if (!hasMoreEntries) {
checkReportsIntegrity(snapDiffJob, idx);
checkReportsIntegrity(snapDiffJob, index, diffReportList.size());
Copy link
Contributor

@hemantk-12 hemantk-12 Jun 8, 2023

Choose a reason for hiding this comment

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

For now, I'm OK to with the check. Because it will eventually fail for the below scenario but it should fail fast.

Case:
SnapshotDiffJob.TotalEntries = 1000, PageSize = 1000, Report iterators says 2500+ (<3000) entries (due to some corruption (or some bug).

At the end of the first page response, checkReportsIntegrity check will be skipped and client will call again for second page. checkReportsIntegrity check will be skipped again at the end of second page response and client will call for third page. At the end of the third page request will fail because eventually it will call checkReportsIntegrity.

@hemantk-12
Copy link
Contributor

hemantk-12 commented Jun 8, 2023 •
edited
Loading

Overall PR looks good to me. The cosmetic changes and this issue can be fixed as separate jira. Please create jira for this.

Please fix the checkstyle and other comment I gave.

On a side note, please resolve the comments if you have fixed or reply to them with justification. Don't just resolve comments without addressing them in one of the above way. For e.g.

  1. https://github.com/apache/ozone/pull/4678/files#r1204811251
  2. https://github.com/apache/ozone/pull/4678/files#r1204812669
  3. https://github.com/apache/ozone/pull/4678/files#r1204817089
  4. HDDS-8477. Unit test for Snapdiff using tombstone entries #4678 (comment)
  5. HDDS-8477. Unit test for Snapdiff using tombstone entries #4678 (comment)

@prashantpogde
Copy link
Contributor

prashantpogde commented Jun 9, 2023

Thank you @swamirishi for this PR.

@prashantpogde prashantpogde merged commit f5e2250 into apache:master Jun 9, 2023
@DaveTeng0 DaveTeng0 mentioned this pull request Jun 9, 2023
Merged
errose28 added a commit to errose28/ozone that referenced this pull request Jun 10, 2023
@errose28
Merge branch 'master' into tmp-dir-refactor
ccded0a 
* master: (73 commits)
  HDDS-8587. Test that CertificateClient can store multiple rootCA certificates (apache#4852)
  HDDS-8801. ReplicationManager: Add metric to count how often replication is throttled (apache#4864)
  HDDS-8477. Unit test for Snapdiff using tombstone entries (apache#4678)
  HDDS-7507. [Snapshot] Implement List Snapshot API Pagination (apache#4065) (apache#4861)
  HDDS-8373. Document that setquota doesn't accept decimals (apache#4856)
  HDDS-8779. Recon - Expose flag for enable/disable of heatmap. (apache#4845)
  HDDS-8677. Ozone admin OM CLI command for block tokens (apache#4760)
  HDDS-8164. Authorize secret key APIs (apache#4597)
  HDDS-7945. Integrate secret keys to SCM snapshot (apache#4549)
  HDDS-8003. E2E integration test cases for block tokens (apache#4547)
  HDDS-7831. Use symmetric secret key to sign and verify token (apache#4417)
  HDDS-7830. SCM API for OM and Datanode to get secret keys (apache#4345)
  HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM (apache#4194)
  HDDS-8679. Add dedicated, configurable thread pool for OM gRPC server (apache#4771)
  HDDS-8790. Split EC acceptance tests (apache#4855)
  HDDS-8714. TestScmHAFinalization: mark testFinalizationWithRestart as flaky, enable other test cases
  HDDS-8787. Reduce ozone sh calls in robot tests (apache#4854)
  HDDS-8774. Log allocation stack trace for leaked CodecBuffer (apache#4840)
  HDDS-8729. Add metric for count of blocks pending deletion on datanode (apache#4800)
  HDDS-8780. Leak of ManagedChannel in HASecurityUtils (apache#4850)
  ...
errose28 added a commit to errose28/ozone that referenced this pull request Jun 10, 2023
@errose28
Merge branch 'tmp-dir-refactor' into improve-volume-scanner
b581bc9 
* tmp-dir-refactor: (73 commits)
  HDDS-8587. Test that CertificateClient can store multiple rootCA certificates (apache#4852)
  HDDS-8801. ReplicationManager: Add metric to count how often replication is throttled (apache#4864)
  HDDS-8477. Unit test for Snapdiff using tombstone entries (apache#4678)
  HDDS-7507. [Snapshot] Implement List Snapshot API Pagination (apache#4065) (apache#4861)
  HDDS-8373. Document that setquota doesn't accept decimals (apache#4856)
  HDDS-8779. Recon - Expose flag for enable/disable of heatmap. (apache#4845)
  HDDS-8677. Ozone admin OM CLI command for block tokens (apache#4760)
  HDDS-8164. Authorize secret key APIs (apache#4597)
  HDDS-7945. Integrate secret keys to SCM snapshot (apache#4549)
  HDDS-8003. E2E integration test cases for block tokens (apache#4547)
  HDDS-7831. Use symmetric secret key to sign and verify token (apache#4417)
  HDDS-7830. SCM API for OM and Datanode to get secret keys (apache#4345)
  HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM (apache#4194)
  HDDS-8679. Add dedicated, configurable thread pool for OM gRPC server (apache#4771)
  HDDS-8790. Split EC acceptance tests (apache#4855)
  HDDS-8714. TestScmHAFinalization: mark testFinalizationWithRestart as flaky, enable other test cases
  HDDS-8787. Reduce ozone sh calls in robot tests (apache#4854)
  HDDS-8774. Log allocation stack trace for leaked CodecBuffer (apache#4840)
  HDDS-8729. Add metric for count of blocks pending deletion on datanode (apache#4800)
  HDDS-8780. Leak of ManagedChannel in HASecurityUtils (apache#4850)
  ...
@swamirishi
Copy link
Contributor Author

swamirishi commented Jun 12, 2023 •
edited
Loading

@aswinshakil @smengcl Can you push the native jar to the maven repo. https://github.com/swamirishi/ozone/actions/runs/5216137947

@ChenSammi
Copy link
Contributor

ChenSammi commented Jun 13, 2023

@aswinshakil , the TestManagedSstFileReader is consistently failing in CI. Could you please take a look?
https://github.com/apache/ozone/actions/runs/5251982605/jobs/9489304922?pr=4859

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hemantk-12 hemantk-12 hemantk-12 left review comments

@smengcl smengcl smengcl left review comments

@adoroszlai adoroszlai adoroszlai requested changes

+1 more reviewer

@DaveTeng0 DaveTeng0 DaveTeng0 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

snapshot https://issues.apache.org/jira/browse/HDDS-6517

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@swamirishi @hemantk-12 @prashantpogde @ChenSammi @adoroszlai @smengcl @DaveTeng0 @aswinshakil