Skip to content

HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. #7012

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
jojochuang merged 6 commits into from
Aug 2, 2024
Merged

HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout version for every PutBlock. #7012

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
280a77d
HDDS-11259. [hsync] DataNode should verify HBASE_SUPPORT layout versi…
jojochuang Jul 31, 2024
99b6274
Address review comments.
jojochuang Aug 1, 2024
038380c
Remove ozone.incremental.chunk.list from ozone-default.xml
jojochuang Aug 2, 2024
7c5b9c2
Replace UnsupportOperationException with StorageContainerException in…
jojochuang Aug 2, 2024
9b94f0e
Do not mark a container unhealthy if client sends unsupported request…
jojochuang Aug 2, 2024
e36387f
Move layout version check earlier to KeyValueHandler for finalizeBlock.
jojochuang Aug 2, 2024
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: 12 additions & 11 deletions ...rvice/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.atomic.AtomicBoolean;

import org.apache.hadoop.hdds.client.BlockID;
import org.apache.hadoop.hdds.conf.ConfigurationSource;
Expand Down Expand Up @@ -65,7 +66,7 @@ public class BlockManagerImpl implements BlockManager {
// Default Read Buffer capacity when Checksum is not present
private final int defaultReadBufferCapacity;
private final int readMappedBufferThreshold;
private boolean incrementalEnabled;
private final AtomicBoolean incrementalEnabled;

/**
* Constructs a Block Manager.
Expand All @@ -81,15 +82,9 @@ public BlockManagerImpl(ConfigurationSource conf) {
this.readMappedBufferThreshold = config.getBufferSize(
ScmConfigKeys.OZONE_CHUNK_READ_MAPPED_BUFFER_THRESHOLD_KEY,
ScmConfigKeys.OZONE_CHUNK_READ_MAPPED_BUFFER_THRESHOLD_DEFAULT);
incrementalEnabled =
incrementalEnabled = new AtomicBoolean(
config.getBoolean(OZONE_CHUNK_LIST_INCREMENTAL,
OZONE_CHUNK_LIST_INCREMENTAL_DEFAULT);
if (incrementalEnabled && !VersionedDatanodeFeatures.isFinalized(
HDDSLayoutFeature.HBASE_SUPPORT)) {
LOG.warn("DataNode has not finalized upgrading to a version that " +
"supports incremental chunk list. Fallback to full chunk list");
incrementalEnabled = false;
}
OZONE_CHUNK_LIST_INCREMENTAL_DEFAULT));
}

@Override
Expand Down Expand Up @@ -162,7 +157,13 @@ public long persistPutBlock(KeyValueContainer container,
}
}

db.getStore().putBlockByID(batch, incrementalEnabled, localID, data,
if (incrementalEnabled.get() && !VersionedDatanodeFeatures.isFinalized(
HDDSLayoutFeature.HBASE_SUPPORT)) {
Copy link
Contributor

@ashishkumar50 ashishkumar50 Aug 1, 2024

Choose a reason for hiding this comment

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

finalizeBlock(..) in BlockManagerImpl can also have this check and return error before finalize.

if (!VersionedDatanodeFeatures.isFinalized(
            HDDSLayoutFeature.HBASE_SUPPORT)) { }

Copy link
Contributor

@ashishkumar50 ashishkumar50 Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But since OM also checks upgrade finalize before and call will fail in OM itself, So in this case client will not call DN and I think this check is not required in DN in case of finalizeBlock.

Copy link
Contributor

@ChenSammi ChenSammi Aug 1, 2024

Choose a reason for hiding this comment

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

Agree. We should add the HDDSLayoutFeature.HBASE_SUPPORT check in finalizeBlock too. Although there is check in OM, the check in DN is also preferred. It's like the double insurance.

LOG.warn("DataNode has not finalized upgrading to a version that " +
"supports incremental chunk list. Fallback to full chunk list");
incrementalEnabled.set(false);
Copy link
Contributor

@ChenSammi ChenSammi Aug 1, 2024

Choose a reason for hiding this comment

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

There is chance that if pubBlock is first called before HDDSLayoutFeature.HBASE_SUPPORT is finalized, then incrementalEnabled will be set as false. In this case, incrementalEnabled will not come back as "true" without the DN restart.

Copy link
Contributor

@ChenSammi ChenSammi Aug 1, 2024

Choose a reason for hiding this comment

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

@jojochuang , while going through the code, I found we have two properties currently for incremental chunk list,
"ozone.client.incremental.chunk.list" // client side configure, default is false. For Hbase ozone client, this should be true. For other application ozone client, this can be false.
"ozone.incremental.chunk.list" // server side configure. Can we remove this property? Because it's default value is false, once HBASE_SUPPORT finalized, user needs to change it's value to true, and then restart the DN for that. Once "HDDSLayoutFeature.HBASE_SUPPORT" feature is finalized, we expect this property always be true, right? So If it can be removed, then we only need to check whether to reject a partial chunk list based on "HDDSLayoutFeature.HBASE_SUPPORT" feature is finalized or not on DN side.

BTW, there is one case, client sends a partial chunk list, server side "HDDSLayoutFeature.HBASE_SUPPORT" feature is not finalized yet, then this client request should fail, looks like DatanodeStoreWithIncrementalChunkList#putBlockByID doesn't cover this case yet.

}
db.getStore().putBlockByID(batch, incrementalEnabled.get(), localID, data,
containerData, endOfBlock);
if (bcsId != 0) {
db.getStore().getMetadataTable().putWithBatch(
Expand Down Expand Up @@ -258,7 +259,7 @@ private void mergeLastChunkForBlockFinalization(BlockID blockId, DBHandle db,
if (blockData.getMetadata().containsKey(INCREMENTAL_CHUNK_LIST)) {
BlockData emptyBlockData = new BlockData(blockId);
emptyBlockData.addMetadata(INCREMENTAL_CHUNK_LIST, "");
db.getStore().putBlockByID(batch, incrementalEnabled, localID,
db.getStore().putBlockByID(batch, incrementalEnabled.get(), localID,
emptyBlockData, kvContainer.getContainerData(), true);
}
}
Expand Down