Skip to content

HDDS-12691. Calculation of committed space in Datanode seems incorrect #8228

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

Merged
siddhantsangwan merged 1 commit into from
Apr 7, 2025
Merged

HDDS-12691. Calculation of committed space in Datanode seems incorrect #8228

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
23 changes: 16 additions & 7 deletions ...er-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import static org.apache.hadoop.ozone.OzoneConsts.STATE;

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.Lists;
import jakarta.annotation.Nullable;
Expand Down Expand Up @@ -226,6 +227,11 @@ public synchronized void setState(ContainerDataProto.State state) {
}
}

@VisibleForTesting
void setCommittedSpace(boolean committedSpace) {
this.committedSpace = committedSpace;
}

Comment on lines +230 to +234
Copy link
Member

@peterxcli peterxcli Apr 4, 2025

Choose a reason for hiding this comment

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

Suggested change
@VisibleForTesting
void setCommittedSpace(boolean committedSpace) {
this.committedSpace = committedSpace;
}
void setCommittedSpace(boolean committedSpace) {
this.committedSpace = committedSpace;
}

This may use in prod code: https://github.com/apache/ozone/pull/8090/files#diff-7ca35266fa44bbb0ba40f06051fe10f52e2da374a7f8650d9dfa9dcde2153e15R465-R468, so could remove the @VisibleForTesting.

Copy link
Contributor Author

@siddhantsangwan siddhantsangwan Apr 4, 2025

Choose a reason for hiding this comment

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

Thanks for the review @peterxcli. I think we should keep the annotation for now, and remove it in the commit (your pr) where it's actually being used in non-test code. That's because until the method is actually being used outside of tests, it's only visible for testing.

Copy link
Member

@peterxcli peterxcli Apr 4, 2025

Choose a reason for hiding this comment

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

Got it, thanks!

/**
* Return's maximum size of the container in bytes.
* @return maxSize in bytes
Expand Down Expand Up @@ -431,20 +437,23 @@ public long getWriteBytes() {
* @param bytes the number of bytes write into the container.
*/
public void incrWriteBytes(long bytes) {
long unused = getMaxSize() - getBytesUsed();

this.writeBytes.addAndGet(bytes);
/*
Increase the cached Used Space in VolumeInfo as it
maybe not updated, DU or DedicatedDiskSpaceUsage runs
periodically to update the Used Space in VolumeInfo.
*/
this.getVolume().incrementUsedSpace(bytes);
// only if container size < max size
if (committedSpace && unused > 0) {
//with this write, container size might breach max size
long decrement = Math.min(bytes, unused);
this.getVolume().incCommittedBytes(0 - decrement);
// Calculate bytes used before this write operation.
// Note that getBytesUsed() already includes the 'bytes' from the current write.
long bytesUsedBeforeWrite = getBytesUsed() - bytes;
Copy link
Contributor

@ChenSammi ChenSammi Apr 7, 2025

Choose a reason for hiding this comment

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

Looks like the overwrite factor is not considered here, though I'm not clear what's the overwrite usage, but according to the current code, if overwrite is true, then bytesUsed will not have bytes added.

Copy link
Contributor

@ChenSammi ChenSammi Apr 7, 2025

Choose a reason for hiding this comment

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

Maybe we should discuss if this OverWriteRequested can be deprecated.

Copy link
Contributor Author

@siddhantsangwan siddhantsangwan Apr 7, 2025

Choose a reason for hiding this comment

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

You're right Sammi. If it's overwrite, it's wrong to increment volume used space and decrease committed space.

It looks like even the old code wasn't handling the overwrite case correctly and there isn't enough test coverage. I'm also not sure how overwrite is used or the concept behind it.

  /**
   * Overwrite is permitted if an only if the user explicitly asks for it. We
   * permit this iff the key/value pair contains a flag called
   * [OverWriteRequested, true].
   *
   * @param chunkInfo - Chunk info
   * @return true if the user asks for it.
   */
  private static boolean isOverWritePermitted(ChunkInfo chunkInfo) {
    String overWrite = chunkInfo.getMetadata(OzoneConsts.CHUNK_OVERWRITE);
    return Boolean.parseBoolean(overWrite);
  }

So it's only permitted if ChunkInfo has explicit metadata that says OverWriteRequested is true.

// Calculate how much space was available within the max size limit before this write
long availableSpaceBeforeWrite = getMaxSize() - bytesUsedBeforeWrite;
if (committedSpace && availableSpaceBeforeWrite > 0) {
// Decrement committed space only by the portion of the write that fits within the originally committed space,
// up to maxSize
long decrement = Math.min(bytes, availableSpaceBeforeWrite);
this.getVolume().incCommittedBytes(-decrement);
}
}

Expand Down
155 changes: 151 additions & 4 deletions ...src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,13 +627,160 @@ public void testWriteChunk(ContainerTestVersionInfo versionInfo)
}

/**
* Writes many chunks of the same block into different chunk files and
* verifies that we have that data in many files.
* Tests that committed space is correctly decremented when a write fits entirely within the available space under
* max container size. It should be decremented by the number of bytes written.
*
* @throws IOException
* @throws NoSuchAlgorithmException
* Before write: Container size is less than max size
* After write: Container size is still less than max size
*/
@ContainerTestVersionInfo.ContainerTest
public void testIncrWriteBytesDecrementsCommittedWhenWriteFits(ContainerTestVersionInfo versionInfo)
Copy link
Member

@peterxcli peterxcli Apr 4, 2025

Choose a reason for hiding this comment

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

nit:
Do you want to parameterize these three test about the commit space boundary test?
Can have two parameters for them (int sizeTillMax, int desiredCommitDecrment), and add description directly on those test sets:

(ContainerTestHelper.CONTAINER_MAX_SIZE, 256)
(100, 100)
(0, 0)
(256, 256)

Copy link
Contributor Author

@siddhantsangwan siddhantsangwan Apr 7, 2025

Choose a reason for hiding this comment

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

In this particular case I find having explicit, different tests to be simpler and more understandable. I think it's good enough for now!

throws IOException {
initSchemaAndVersionInfo(versionInfo);
BlockID blockID = ContainerTestHelper.
getTestBlockID(getTestContainerID());
long testContainerID = blockID.getContainerID();
Container container = containerSet.getContainer(testContainerID);
if (container == null) {
container = addContainer(containerSet, testContainerID);
}

long commitBytesBefore = container.getContainerData().getVolume().getCommittedBytes();
long writeBytes = 256 * OzoneConsts.MB;
container.getContainerData().updateWriteStats(writeBytes, false);
long commitBytesAfter = container.getContainerData().getVolume().getCommittedBytes();
long commitDecrement = commitBytesBefore - commitBytesAfter;
// did we decrement commit bytes by the amount of data we wrote?
assertEquals(writeBytes, commitDecrement);
}

/**
* Tests the scenario where a write operation causes the container usage to exceed max container size.
* The committed space should only be decremented by the amount of the write that fit within the max limit.
*
* Before Write: Container size is within max size
* After Write: Container size exceeds max size
*/
@ContainerTestVersionInfo.ContainerTest
public void testIncrWriteBytesDecrementsCommittedCorrectlyWhenWriteExceedsMax(ContainerTestVersionInfo versionInfo)
throws IOException {
initSchemaAndVersionInfo(versionInfo);
BlockID blockID = ContainerTestHelper.
getTestBlockID(getTestContainerID());
long testContainerID = blockID.getContainerID();
Container container = containerSet.getContainer(testContainerID);
if (container == null) {
container = addContainer(containerSet, testContainerID);
}

// fill the container close to the max size first
long writeBytes = container.getContainerData().getMaxSize() - 100;
container.getContainerData().updateWriteStats(writeBytes, false);

// the next write will make the container size exceed max size
long commitBytesBefore = container.getContainerData().getVolume().getCommittedBytes();
writeBytes = 256 * OzoneConsts.MB;
container.getContainerData().updateWriteStats(writeBytes, false);
long commitBytesAfter = container.getContainerData().getVolume().getCommittedBytes();
long commitDecrement = commitBytesBefore - commitBytesAfter;
// did we decrement commit bytes by the amount that was remaining, ie, 100 bytes?
assertEquals(100, commitDecrement);
}

/**
* Tests that committed space is not decremented if the container was already
* full (or overfull) before the write operation.
*/
@ContainerTestVersionInfo.ContainerTest
public void testIncrWriteBytesDoesNotDecrementCommittedWhenContainerFull(ContainerTestVersionInfo versionInfo)
throws IOException {
initSchemaAndVersionInfo(versionInfo);
BlockID blockID = ContainerTestHelper.
getTestBlockID(getTestContainerID());
long testContainerID = blockID.getContainerID();
Container container = containerSet.getContainer(testContainerID);
if (container == null) {
container = addContainer(containerSet, testContainerID);
}

// fill the container completely first
long writeBytes = container.getContainerData().getMaxSize();
container.getContainerData().updateWriteStats(writeBytes, false);

// the next write will make the container size exceed max size
long commitBytesBefore = container.getContainerData().getVolume().getCommittedBytes();
writeBytes = 256 * OzoneConsts.MB;
container.getContainerData().updateWriteStats(writeBytes, false);
long commitBytesAfter = container.getContainerData().getVolume().getCommittedBytes();
long commitDecrement = commitBytesBefore - commitBytesAfter;
// decrement should be 0, as the container was already full before the write
assertEquals(0, commitDecrement);
}

/**
* In this test, the write fills the container exactly to max size. Committed space should be decremented by the
* amount of bytes written.
*/
@ContainerTestVersionInfo.ContainerTest
public void testIncrWriteBytesDecrementsCommittedCorrectlyWhenWrittenToBrim(ContainerTestVersionInfo versionInfo)
throws IOException {
initSchemaAndVersionInfo(versionInfo);
BlockID blockID = ContainerTestHelper.
getTestBlockID(getTestContainerID());
long testContainerID = blockID.getContainerID();
Container container = containerSet.getContainer(testContainerID);
if (container == null) {
container = addContainer(containerSet, testContainerID);
}

// fill the container completely first
long writeBytes = container.getContainerData().getMaxSize() - 256 * OzoneConsts.MB;
container.getContainerData().updateWriteStats(writeBytes, false);

// the next write will make the container size exactly equal to max size
long commitBytesBefore = container.getContainerData().getVolume().getCommittedBytes();
writeBytes = 256 * OzoneConsts.MB;
Copy link
Contributor

@ChenSammi ChenSammi Apr 7, 2025

Choose a reason for hiding this comment

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

Container max size is defined by CONTAINER_MAX_SIZE, which is 1GB.

Copy link
Contributor Author

@siddhantsangwan siddhantsangwan Apr 7, 2025

Choose a reason for hiding this comment

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

After the write at line 737, only 256MB is remaining out of 1GB.

container.getContainerData().updateWriteStats(writeBytes, false);
long commitBytesAfter = container.getContainerData().getVolume().getCommittedBytes();
long commitDecrement = commitBytesBefore - commitBytesAfter;
// decrement should be writeBytes, as the write does not exceed max size
assertEquals(writeBytes, commitDecrement);
}

/**
* Commited space should not change when committedSpace is set to false.
*/
@ContainerTestVersionInfo.ContainerTest
public void testIncrWriteBytesDoesNotChangeCommittedSpaceWhenItsDisabled(ContainerTestVersionInfo versionInfo)
throws IOException {
initSchemaAndVersionInfo(versionInfo);
BlockID blockID = ContainerTestHelper.
getTestBlockID(getTestContainerID());
long testContainerID = blockID.getContainerID();
Container container = containerSet.getContainer(testContainerID);
if (container == null) {
container = addContainer(containerSet, testContainerID);
}

// For setup, we need to set committedSpace to false first
container.getContainerData().setCommittedSpace(false);
long commitBytesBefore = container.getContainerData().getVolume().getCommittedBytes();
long writeBytes = 256 * OzoneConsts.MB;
container.getContainerData().updateWriteStats(writeBytes, false);
long commitBytesAfter = container.getContainerData().getVolume().getCommittedBytes();
long commitDecrement = commitBytesBefore - commitBytesAfter;
// decrement should be 0, as commit space is disabled for this container
assertEquals(0, commitDecrement);
}

/**
* Writes many chunks of the same block into different chunk files and
* verifies that we have that data in many files.
*
* @throws IOException
* @throws NoSuchAlgorithmException
*/
@ContainerTestVersionInfo.ContainerTest
public void testWritReadManyChunks(ContainerTestVersionInfo versionInfo)
throws IOException {
initSchemaAndVersionInfo(versionInfo);
Expand Down