Skip to content

HDDS-13075. Fix default value in description of container placement policy configs #8511

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

Merged
merged 3 commits into from
May 28, 2025

Conversation

sreejasahithi
Copy link
Contributor

What changes were proposed in this pull request?

The description of ozone.scm.container.placement.impl and ozone.scm.container.placement.ec.impl has been updated to show that the default property used is SCMContainerPlacementRackAware

What is the link to the Apache JIRA

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

How was this patch tested?

https://github.com/sreejasahithi/ozone/actions/runs/15186737895

…mpl and ozone.scm.container.placement.ec.impl
@sreejasahithi sreejasahithi marked this pull request as ready for review May 27, 2025 04:08
Copy link
Contributor

@ashishkumar50 ashishkumar50 left a comment

Choose a reason for hiding this comment

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

@sreejasahithi Thanks for the patch, Please find below comments.
Also please update default value in code with SCMContainerPlacementRackAware.

@@ -928,7 +928,7 @@
The full name of class which implements
org.apache.hadoop.hdds.scm.PlacementPolicy.
The class decides which datanode will be used to host the container replica in EC mode. If not set,
org.apache.hadoop.hdds.scm.container.placement.algorithms.SCMContainerPlacementRandom will be used as default
org.apache.hadoop.hdds.scm.container.placement.algorithms.SCMContainerPlacementRackAware will be used as default
Copy link
Contributor

Choose a reason for hiding this comment

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

SCMContainerPlacementRackScatter will be used as default.

@@ -915,8 +915,8 @@
<description>
The full name of class which implements
org.apache.hadoop.hdds.scm.PlacementPolicy.
The class decides which datanode will be used to host the container replica. If not set,
org.apache.hadoop.hdds.scm.container.placement.algorithms.SCMContainerPlacementRandom will be used as default
The class decides which datanode will be used to host the container replica.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep "If not set,"

@adoroszlai adoroszlai changed the title HDDS-13075. Update the description of ozone.scm.container.placement.impl and ozone.scm.container.placement.ec.impl HDDS-13075. Fix default value in description of container placement policy configs May 27, 2025
Copy link
Contributor

@sarvekshayr sarvekshayr left a comment

Choose a reason for hiding this comment

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

Hi @sreejasahithi.
Can we remove this TODO now that we've changed the default placement policy for EC?

// TODO: Change default placement policy for EC
final Class<? extends PlacementPolicy> placementClass = conf
.getClass(ScmConfigKeys.OZONE_SCM_CONTAINER_PLACEMENT_EC_IMPL_KEY,
OZONE_SCM_CONTAINER_PLACEMENT_EC_IMPL_DEFAULT,
PlacementPolicy.class);

@sreejasahithi sreejasahithi requested a review from sarvekshayr May 28, 2025 10:28
Copy link
Contributor

@sarvekshayr sarvekshayr left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all the comments. LGTM.

Copy link
Contributor

@ashishkumar50 ashishkumar50 left a comment

Choose a reason for hiding this comment

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

LGTM

@adoroszlai adoroszlai merged commit 08c0ab8 into apache:master May 28, 2025
42 checks passed
@adoroszlai
Copy link
Contributor

Thanks @sreejasahithi for the patch, @ashishkumar50, @sarvekshayr for the review.

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