Skip to content

HDDS-9039. Added a test to verify that no compaction log entry is added to compactionLogTable and DAG when tarball creation is in progress. #6171

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

Closed
hemantk-12 wants to merge 1 commit into from
Closed

HDDS-9039. Added a test to verify that no compaction log entry is added to compactionLogTable and DAG when tarball creation is in progress. #6171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...b-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ void addToCompactionLogTable(CompactionLogEntry compactionLogEntry) {
* Check if there is any in_progress tarball creation request and wait till
* all tarball creation finish, and it gets notified.
*/
private void waitForTarballCreation() {
private synchronized void waitForTarballCreation() {
Copy link
Contributor

@swamirishi swamirishi Feb 6, 2024

Choose a reason for hiding this comment

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

How would this change help in anyway? I don't think rocksdb will do multiple compaction in parallel. onCompactionCompleted method will commit the compaction.

Copy link
Contributor

@swamirishi swamirishi Feb 6, 2024

Choose a reason for hiding this comment

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

I believe we would need a lock on the tarballRequestCount when incrementing and decrementing request and add the compaction log entry within the lock.

Copy link
Contributor

@swamirishi swamirishi Feb 6, 2024

Choose a reason for hiding this comment

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

waitForTarballCreation wouldn't really ensure the count would be zero after the method call. There could be a race condition here.

Copy link
Contributor Author

@hemantk-12 hemantk-12 Feb 6, 2024

Choose a reason for hiding this comment

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

tarballRequestCount is an AtomicInteger so the incrementing and decrementing it would be atomic.

waitForTarballCreation is synchronized because wait/notify are supposed to be allowed on the same object's lock otherwise it throws IllegalMonitorStateException. Doc: wait/notifyAll.

More in discussion on jira: HDDS-9039 why we are adding the test this way.

Copy link
Contributor

@swamirishi swamirishi Apr 17, 2024

Choose a reason for hiding this comment

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

What I didn't understand is getCheckpoint() method is not taking a synchronized lock. So the question if waitForTarballCreation() passes.

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java

Line 248 in 2c0580d

differ.incrementTarballRequestCount();
can still run. Thus isn't there a race condition here? So while compaction is running we still end up starting a tarballing for the checkpoint.

Copy link
Contributor Author

@hemantk-12 hemantk-12 Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think you are right if we are appending compaction log entries to a txt file but we don't use that anymore and add entries to compactionLog columnFamily.

I gave it more thought and I think this locking is not needed anymore because we use RocksDB column family for compaction now. So compaction entry will be either present in the table or not in the snapshot of the ActiveFS depending on the order of compaction entry append and checkpoint creation. Hence we can simply remove this locking code and just rely on RocksDB.

Original discussion to add lock: #4680 (comment)

Copy link
Contributor

@swamirishi swamirishi Apr 17, 2024

Choose a reason for hiding this comment

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

yeah i agree we don't need the locks since we are writing it into a rocksdb table. This is fine as long as we are using rocksdb checkpoints to take a checkpoint of the rocksdb.

while (tarballRequestCount.get() != 0) {
try {
wait(Integer.MAX_VALUE);
Expand Down
50 changes: 50 additions & 0 deletions ...eckpoint-differ/src/test/java/org/apache/ozone/rocksdiff/TestRocksDBCheckpointDiffer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1947,4 +1947,54 @@ public void testShouldSkipFile(String description,
assertEquals(expectedResult, rocksDBCheckpointDiffer
.shouldSkipCompaction(columnFamilyBytes, inputFiles, outputFiles));
}

/**
* Test to verify that no compaction log entry is added to compactionLogTable and DAG.
*/
@Test
void testCompactionLogsAreNotAppendedIfTarballCreationIsInProgress() throws Exception {
rocksDBCheckpointDiffer.setSnapshotInfoTableCFHandle(keyTableCFHandle);

// Increment tarball request count twice to mimic that two tarball requests are in progress,
// so the compaction thread waits.
rocksDBCheckpointDiffer.incrementTarballRequestCount();
rocksDBCheckpointDiffer.incrementTarballRequestCount();
assertEquals(2, rocksDBCheckpointDiffer.getTarballRequestCount());

writeKeysAndCheckpointing();
Thread.sleep(1000);

// Verify that no log entry gets added to compactionLogTable and DAG when tarball request count is not zero.
assertCompactionTableAndDagEntries(0, 0, 0);

// Release one tarball request by decrementing tarball request count.
rocksDBCheckpointDiffer.decrementTarballRequestCountAndNotify();
assertEquals(1, rocksDBCheckpointDiffer.getTarballRequestCount());

Thread.sleep(1000);

// Verify again that no log entry gets added to compactionLogTable and DAG after decrementing tarball request count,
// but it is still non-zero.
assertCompactionTableAndDagEntries(0, 0, 0);

// Release the second tarball request by decrementing tarball request count again,
// so the compaction thread can continue.
rocksDBCheckpointDiffer.decrementTarballRequestCountAndNotify();

assertEquals(0, rocksDBCheckpointDiffer.getTarballRequestCount());
GenericTestUtils.waitFor(() -> countEntriesInCompactionLogTable() == 1, 100, 1000);

// Verify that log entries get added to compactionLogTable and DAG after only tarball request count reaches zero.
assertCompactionTableAndDagEntries(1, 5, 4);
cleanUpSnapshots();
}

private void assertCompactionTableAndDagEntries(int expectedEntriesInCompactionLogTable,
int expectedNodesInCompactionDag,
int expectedArcsInCompactionDag) {

assertEquals(expectedEntriesInCompactionLogTable, countEntriesInCompactionLogTable());
assertEquals(expectedNodesInCompactionDag, rocksDBCheckpointDiffer.getForwardCompactionDAG().nodes().size());
assertEquals(expectedArcsInCompactionDag, rocksDBCheckpointDiffer.getForwardCompactionDAG().edges().size());
}
}