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.

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

HDDS-8838. Update default datanode check empty containter on disk to false #4937

Merged
merged 2 commits into from
Jun 22, 2023
Merged

HDDS-8838. Update default datanode check empty containter on disk to false #4937

merged 2 commits into from
Jun 22, 2023

Conversation

ashishkumar50
Copy link
Contributor

What changes were proposed in this pull request?

Currently we are checking both rocks db as well as disk to determine whether container is really empty or not. There are certain scenario where blocks remains on disk and causing container delete fail forever.
More details is in the Jira.
In this PR we are changing default value of hdds.datanode.check.empty.container.on-disk.on.delete to false, which will make container empty check just depends on rocksdb by default.

What is the link to the Apache JIRA

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

How was this patch tested?

Verified using new integration test and with existing test.

@ashishkumar50
Copy link
Contributor Author

@sodonnel @errose28, Please help to review, Thanks.

sodonnel
sodonnel approved these changes Jun 20, 2023
View reviewed changes
Copy link
Contributor

@sodonnel sodonnel left a comment

Choose a reason for hiding this comment

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

LGTM - I see there is also a test "testDeleteNonEmptyContainerDir" which covers when the flag is set to true, so both scenarios are checked.

errose28
errose28 reviewed Jun 20, 2023
View reviewed changes
Copy link
Contributor

@errose28 errose28 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 quick change @ashishkumar50. Functionally the change looks good. Could we maybe change the names of the two tests and add javadoc explaining that they are testing the true and false values of this config? It makes sense now but the distinction may not be clear to later readers.

...c/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java Outdated Show resolved Hide resolved
@ashishkumar50
Copy link
Contributor Author

Thanks for the quick change @ashishkumar50. Functionally the change looks good. Could we maybe change the names of the two tests and add javadoc explaining that they are testing the true and false values of this config? It makes sense now but the distinction may not be clear to later readers.

Thanks @errose28, I have updated javadoc and test name to have clear understanding.

@sodonnel
Copy link
Contributor

All checks are green @errose28 are you happy to commit the latest revision?

errose28
errose28 approved these changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@errose28 errose28 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 updates @ashishkumar50 LGTM as well.

@errose28 errose28 merged commit 30f3dc0 into apache:master Jun 22, 2023
errose28 added a commit to errose28/ozone that referenced this pull request Jun 25, 2023
@errose28
Merge branch 'master' into tmp-dir-refactor
e502a07 
* master: (96 commits)
  HDDS-8586 Recon. - API for Count of deletePending keys and amount of data mapped to such keys. (apache#4923)
  HDDS-8908. Intermittent failure in TestBlockDeletion#testBlockDeletion (apache#4958)
  HDDS-8910. Replace LockManager with striped lock in ContainerStateManager (apache#4962)
  HDDS-8917. Move protobuf conversion out of the lock in PipelineStateManagerImpl (apache#4965)
  HDDS-8825. Use apache/hadoop 3.3.5 docker image (apache#4963)
  HDDS-8906. Avoid stream when getting in-service healthy nodes (apache#4960)
  HDDS-8907. Store volume count when storage report is updated (apache#4957)
  HDDS-8905. PipelineManager metrics should not be synchronized (apache#4959)
  HDDS-8553. Improve scanner integration tests. (apache#4936)
  HDDS-8854. Avoid unnecessary DatanodeDetails creation for NodeStateManager lookup (apache#4925)
  HDDS-8315. [Snapshot] Added unit tests for SnapshotDiffManager (apache#4716)
  HDDS-7968. [Snapshot] Improve KeyDeletingService to reclaim eligible key blocks in snapshot's deletedTable (apache#4935)
  HDDS-8838. Update default datanode check empty containter on disk to false (apache#4937)
  HDDS-8763. Support RocksDB iterator with ByteBuffer. (apache#4942)
  HDDS-8543. FSO directory should reflect bucket/cluster default replication (apache#4947)
  HDDS-8898. Replication limit should not be less than reconstruction weight (apache#4954)
  HDDS-8739. Snapdiff should return complete absolute path in Diff Entry (apache#4823)
  HDDS-8908. Mark TestBlockDeletion#testBlockDeletion as flaky
  HDDS-8534. Support asynchronous service logging (apache#4663)
  HDDS-8879. Cleanup SecurityConfig and related class initialization (apache#4921)
  ...
errose28 added a commit to errose28/ozone that referenced this pull request Jun 26, 2023
@errose28
Merge branch 'tmp-dir-refactor' into improve-volume-scanner
fa4207c 
* tmp-dir-refactor: (99 commits)
  HDDS-8586 Recon. - API for Count of deletePending keys and amount of data mapped to such keys. (apache#4923)
  Fix SCM HA finalization compat test
  HDDS-8908. Intermittent failure in TestBlockDeletion#testBlockDeletion (apache#4958)
  HDDS-8910. Replace LockManager with striped lock in ContainerStateManager (apache#4962)
  HDDS-8917. Move protobuf conversion out of the lock in PipelineStateManagerImpl (apache#4965)
  HDDS-8825. Use apache/hadoop 3.3.5 docker image (apache#4963)
  HDDS-8906. Avoid stream when getting in-service healthy nodes (apache#4960)
  HDDS-8907. Store volume count when storage report is updated (apache#4957)
  HDDS-8905. PipelineManager metrics should not be synchronized (apache#4959)
  HDDS-8553. Improve scanner integration tests. (apache#4936)
  HDDS-8854. Avoid unnecessary DatanodeDetails creation for NodeStateManager lookup (apache#4925)
  HDDS-8315. [Snapshot] Added unit tests for SnapshotDiffManager (apache#4716)
  HDDS-7968. [Snapshot] Improve KeyDeletingService to reclaim eligible key blocks in snapshot's deletedTable (apache#4935)
  HDDS-8838. Update default datanode check empty containter on disk to false (apache#4937)
  HDDS-8763. Support RocksDB iterator with ByteBuffer. (apache#4942)
  HDDS-8543. FSO directory should reflect bucket/cluster default replication (apache#4947)
  HDDS-8898. Replication limit should not be less than reconstruction weight (apache#4954)
  HDDS-8739. Snapdiff should return complete absolute path in Diff Entry (apache#4823)
  HDDS-8908. Mark TestBlockDeletion#testBlockDeletion as flaky
  HDDS-8534. Support asynchronous service logging (apache#4663)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sodonnel sodonnel sodonnel approved these changes

@errose28 errose28 errose28 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ashishkumar50 @sodonnel @errose28