-
Notifications
You must be signed in to change notification settings - Fork 477
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-3179 Pipeline placement based on Topology does not have fallback #678
Conversation
...ds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
Outdated
Show resolved
Hide resolved
9342cb5
to
b007fae
Compare
...ds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
Outdated
Show resolved
Hide resolved
...ds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
Outdated
Show resolved
Hide resolved
...ds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
Outdated
Show resolved
Hide resolved
6696d42
to
c4576bc
Compare
...ds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
Outdated
Show resolved
Hide resolved
...ds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
Show resolved
Hide resolved
...erver-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelinePlacementPolicy.java
Show resolved
Hide resolved
4e9ce5b
to
b7e6aff
Compare
Acceptance is failing with:
This is known issue and will be fixed soon - HDDS-3284. Integration tests are flaky. it-Freon:
One of these issues covers it: https://issues.apache.org/jira/browse/HDDS-3266 and it-freon: https://issues.apache.org/jira/browse/HDDS-3257 it-client I am not sure |
...ds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
Outdated
Show resolved
Hide resolved
...erver-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelinePlacementPolicy.java
Outdated
Show resolved
Hide resolved
Thanks for the updates here. I think the code looks much cleaner now with the debug statements and refactored block in getResultSet(). There are just a couple of minor changes needed to finish this one off. |
Thanks for the detailed review. It really helps. @sodonnel |
Thanks for quickly addressing the final issues. I am +1 on this now. I will commit it later pending the CI checks looking good. |
All tests are green except this one, which I have seen fail in several other PRs, so it is flaky:
|
If you see a flaky test, please disable it (without and issue) + create a new open issue and repeat the build. There is some risk that one flakiness hides a other. Merging patches without green build makes harder to debug flaky tests. |
What changes were proposed in this pull request?
When rack awareness and topology is enabled, pipeline placement can fail when there is only one node on the rack.
Should add fall back logic to search for nodes from other racks.
(Please fill in changes proposed in this fix)
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3179
(Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HDDS-XXXX. Fix a typo in YYY.)
Please replace this section with the link to the Apache JIRA)
How was this patch tested?
UT
(Please explain how this patch was tested. Ex: unit tests, manual tests)
(If this patch involves UI changes, please attach a screen-shot; otherwise, remove this)