Skip to content
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.

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

HDDS-12081. TestKeyInputStream repeats tests with default container layout #7704

Merged
merged 3 commits into from
Jan 24, 2025

Conversation

adoroszlai
Copy link
Contributor

What changes were proposed in this pull request?

Problem:
TestChunkInputStream and TestKeyInputStream repeat tests with default container layout, instead of testing with both FILE_PER_BLOCK and FILE_PER_CHUNK.

Causes:

  • The tests set wrong string representation of the configured container layout (since HDDS-4552). Test cluster always falls back to the default layout, FILE_PER_BLOCK.
  • KeyValueHandler does not allow creating containers with FILE_PER_CHUNK layout, even if configured correctly (since HDDS-11753).

Fix:

  • Change the tests to start the cluster only once, with the default layout.
  • Set the layout in datanode configs right before creating data.
  • Close existing containers afterwards, to ensure each test writes data to new container, created with the layout being tested.

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

How was this patch tested?

Added temporary log in KeyValueHandler.handleCreateContainer to show the layout being used.

ContainerLayoutVersion layoutVersion =
ContainerLayoutVersion.getConfiguredVersion(conf);
KeyValueContainerData newContainerData = new KeyValueContainerData(
containerID, layoutVersion, maxContainerSize, request.getPipelineID(),
getDatanodeId());

CI:
https://github.com/adoroszlai/ozone/actions/runs/12783565176

@adoroszlai adoroszlai self-assigned this Jan 15, 2025
@adoroszlai adoroszlai requested a review from jojochuang January 16, 2025 03:42
scm.getContainerManager().getContainers().forEach(container -> {
if (container.isOpen()) {
try {
TestHelper.waitForContainerClose(getCluster(), container.getContainerID());
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this.
Once open containers are closed, SCM will recreate open containers. Containers will be created using the layout specified at the DataNodes, which doesn't get updated until the start of the next test.

Wouldn't it make more sense to call closeContainers() after updateConfig()?

Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

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

Also there are a few other tests still using FILE_PER_CHUNK. TestFinalizeBlock is one of them. Should it get rid of duplicate tests that reference FILE_PER_CHUNK?

@adoroszlai
Copy link
Contributor Author

Thanks @jojochuang for the review, good points. I have updated the patch. (I don't intend to remove usage of FILE_PER_CHUNK from unit tests.)

Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

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

+1

@jojochuang jojochuang merged commit 1bd721b into apache:master Jan 24, 2025
29 checks passed
@jojochuang
Copy link
Contributor

Merged. Thanks @adoroszlai for quick update.

@adoroszlai adoroszlai deleted the HDDS-12081 branch January 25, 2025 04:59
@adoroszlai
Copy link
Contributor Author

Thanks @jojochuang for reviewing and merging this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants