Skip to content

Comments

HDDS-7158. ldb cli command supports to scan container V3.#3705

Merged
ChenSammi merged 6 commits intoapache:masterfrom
ChenSammi:HDDS-7158
Aug 26, 2022
Merged

HDDS-7158. ldb cli command supports to scan container V3.#3705
ChenSammi merged 6 commits intoapache:masterfrom
ChenSammi:HDDS-7158

Conversation

@ChenSammi
Copy link
Contributor

@kerneltime kerneltime requested a review from guihecheng August 22, 2022 16:17
@kerneltime
Copy link
Contributor

@DuongNguyen0

Copy link
Contributor

@duongkame duongkame 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 @ChenSammi. This looks good to me, only left a few nitpick comments.


@Override
protected void finalize() throws Throwable {
ManagedRocksObjectMetrics.INSTANCE.increaseManagedObject();
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 make it an util in ManagedRocksObjectMetrics.

static void assertClosed(RocksMutableObject rocksObject) {
    ManagedRocksObjectMetrics.INSTANCE.increaseManagedObject();
    if (rocksObject.isOwningHandle()) {
      ManagedRocksObjectMetrics.INSTANCE.increaseLeakObject();
      LOG.warn("{} is not closed properly",
          rocksObject.getClass().getSimpleName());
    }
  }

Copy link
Contributor Author

@ChenSammi ChenSammi Aug 24, 2022

Choose a reason for hiding this comment

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

@DuongNguyen0 , thanks for the code review. RocksMutableObject.isOwningHandle has a protected access privilege, and cannot be directly called in either ManagedRocksObjectMetrics or ManagedRocksObjectUtils(I assume you mean this class). That's why move the check into the final method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, thanks for the clarification, Sammi.

if (schemaV3) {
int index =
DatanodeSchemaThreeDBDefinition.getContainerKeyPrefixLength();
String cid = ((String)key).substring(0, index);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
String cid = ((String)key).substring(0, index);
String cid = key.toString().substring(0, index);

int index =
DatanodeSchemaThreeDBDefinition.getContainerKeyPrefixLength();
String cid = ((String)key).substring(0, index);
String blockId = ((String)key).substring(index);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
String blockId = ((String)key).substring(index);
String blockId = key.toString().substring(index);

import org.rocksdb.Slice;

/**
* Managed Options.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Managed Options.
* Managed Slice.

Copy link
Contributor

@guihecheng guihecheng left a comment

Choose a reason for hiding this comment

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

LGTM+1
Thanks @ChenSammi !

@ChenSammi
Copy link
Contributor Author

Thanks @DuongNguyen0 and @guihecheng for the code review.

@ChenSammi ChenSammi merged commit 48230b6 into apache:master Aug 26, 2022
@ChenSammi ChenSammi deleted the HDDS-7158 branch February 20, 2023 03:31
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.

4 participants