Skip to content

HDDS-7851. Apply flush and compact on all column families#4219

Merged
kerneltime merged 4 commits intoapache:masterfrom
symious:HDDS-7851
Feb 6, 2023
Merged

HDDS-7851. Apply flush and compact on all column families#4219
kerneltime merged 4 commits intoapache:masterfrom
symious:HDDS-7851

Conversation

@symious
Copy link
Contributor

@symious symious commented Jan 30, 2023

What changes were proposed in this pull request?

The operation of flush and compact only works for "default" column family.

This ticket is to also apply the operations on other column families.

What is the link to the Apache JIRA

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

How was this patch tested?

unit test.

@symious
Copy link
Contributor Author

symious commented Jan 30, 2023

@ChenSammi Could you help to review the PR?

@kerneltime
Copy link
Contributor

Should the column family be flushed when flushing the wal as well? cc @errose28 @duongkame

@kerneltime
Copy link
Contributor

Thank you at @symious for fixing this issue.

@symious
Copy link
Contributor Author

symious commented Jan 30, 2023

Should the column family be flushed when flushing the wal as well?

@kerneltime Thank you for the review. Are your refering to the function of "db.flushWal(bool)"? I think the wal is shared by all column families, should be no need of this operation.

@kerneltime
Copy link
Contributor

@GeorgeJahad can you please take a look?

@duongkame
Copy link
Contributor

Should the column family be flushed when flushing the wal as well?

@kerneltime Thank you for the review. Are your refering to the function of "db.flushWal(bool)"? I think the wal is shared by all column families, should be no need of this operation.

I think WAL is flushed (to OS cache) automatically after each writes to achieve durability (unless Ozone has a config to disable it).

On the other hand, this PR deals with flushing column families' memtables to SST files to persist the compaction after a snapshot is taken. We already flush memtable of the default column family so it makes sense to flush all the column families as well.

@sadanand48
Copy link
Contributor

Some of these methods are already implemented in the snapshot branch (HDDS-6517). Please take a look.

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.

overall LGTM.

<allowedImport>org.rocksdb.StatsLevel</allowedImport>
<allowedImport>org.rocksdb.TransactionLogIterator.BatchResult</allowedImport>
<allowedImport>org.rocksdb.TickerType</allowedImport>
<allowedImport>org.rocksdb.LiveFileMetaData</allowedImport>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it doesn't look like this class is used anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@duongkame Thank you for the review, removed the unused entry, please have a look.

symious and others added 2 commits February 2, 2023 09:40
…ls/db/managed/ManagedCompactRangeOptions.java

Co-authored-by: Duong Nguyen <duongnt.is@gmail.com>
@symious
Copy link
Contributor Author

symious commented Feb 2, 2023

Some of these methods are already implemented in the snapshot branch (HDDS-6517). Please take a look.

@sadanand48 Thank you for the review. I think it should be easy to resolve the conflicts, right?

@sadanand48
Copy link
Contributor

Thank you for the review. I think it should be easy to resolve the conflicts, right?

The snapshot branch got merged to master today, The conflicts need to be resolved from this PR now.

@kerneltime kerneltime merged commit 3ca0a98 into apache:master Feb 6, 2023
hemantk-12 pushed a commit to hemantk-12/ozone that referenced this pull request Feb 16, 2023
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