HDFS-16477. [SPS]: Add metric PendingSPSPaths for getting the number of paths to be processed by SPS #4009
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Hi @jojochuang @Hexiaoqiao , PTAL. Thanks. |
|
Hi @tasanuma @ayushtkn @virajith @virajjasani , could you please also take a look at this? Thanks. |
|
💔 -1 overall
This message was automatically generated. |
|
Unit tests failed because of OOM, regardless of changes. |
ayushtkn
reviewed
Mar 4, 2022
...fs/src/main/java/org/apache/hadoop/hdfs/server/namenode/sps/StoragePolicySatisfyManager.java
Outdated
Show resolved
Hide resolved
...s-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
Outdated
Show resolved
Hide resolved
Thank you @ayushtkn very much for the review. I will update it ASAP. |
|
💔 -1 overall
This message was automatically generated. |
|
Hi @tasanuma @virajjasani , could you please also take a look. Thanks. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Hi @ayushtkn , could you please take a look at this? Thank you. |
ayushtkn
reviewed
Mar 26, 2022
...bf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/NamenodeStatusReport.java
Show resolved
Hide resolved
Comment on lines
444
to
498
| dfs.satisfyStoragePolicy(new Path(FILE)); | ||
| // Assert metrics. | ||
| assertEquals(1, hdfsCluster.getNamesystem().getPendingSPSPaths()); | ||
| // Wait till namenode notified about the block location details | ||
| DFSTestUtil.waitExpectedStorageType(FILE, StorageType.ARCHIVE, 3, 35000, | ||
| dfs); |
There was a problem hiding this comment.
Is there a race condition possible, if the storage policy gets satisfied before we get the pending sps paths? in that case the assertion shall fail I suppose?
There was a problem hiding this comment.
Is there a race condition possible, if the storage policy gets satisfied before we get the pending sps paths? in that case the assertion shall fail I suppose?
Thanks for your review. The method doTestWhenStoragePolicySetToCOLD is only called in one place. I think there should be no race condition? What do you think of this?
There was a problem hiding this comment.
// Wait till namenode notified about the block location details
DFSTestUtil.waitExpectedStorageType(FILE, StorageType.ARCHIVE, 3, 35000,
dfs);
Here you are waiting for SPS to process the path and move the blocks to the correct place, once this is done, whether getPendingSPSPaths will still return 1? I suppose no, right? the path got processed so the count should reduce to 0.
So, my take is you don't have a control on DFSTestUtil.waitExpectedStorageType(FILE, StorageType.ARCHIVE, 3, 35000, dfs);, if by chance SPS process that path before your assertion then the test will fail.
I haven't gone through the code, but that is what I felt in my initial pass, if it doesn't work this way do lemme know
There was a problem hiding this comment.
// Wait till namenode notified about the block location details DFSTestUtil.waitExpectedStorageType(FILE, StorageType.ARCHIVE, 3, 35000, dfs);Here you are waiting for SPS to process the path and move the blocks to the correct place, once this is done, whether
getPendingSPSPathswill still return 1? I suppose no, right? the path got processed so the count should reduce to 0.So, my take is you don't have a control on
DFSTestUtil.waitExpectedStorageType(FILE, StorageType.ARCHIVE, 3, 35000, dfs);, if by chance SPS process that path before your assertion then the test will fail.I haven't gone through the code, but that is what I felt in my initial pass, if it doesn't work this way do lemme know
Thank you @ayushtkn for your detailed explanation. I made a mistake and you are right. I'll add a new unit test to assert metrics without running SPS.
|
💔 -1 overall
This message was automatically generated. |
|
Hi @ayushtkn , the failed unit test is unrelated to the change. Please have a look again. Thank you very much. |
ayushtkn
approved these changes
Mar 30, 2022
There was a problem hiding this comment.
Some minor stuff with the test else the prod changes LGTM.
Give a check to the Jenkins warning. The cc warnings if they aren't related.
TestRouterDistCpProcedure seems to be failing with timeout. Doesn't look related. If you have bandwidth can track & increase the timeout for it in a separate jira
Comment on lines
495
to
496
| // Assert metrics. | ||
| assertEquals(1, hdfsCluster.getNamesystem().getPendingSPSPaths()); |
There was a problem hiding this comment.
it is still there, you added a new test as well to get rid of this only right?
There was a problem hiding this comment.
Thanks for your comments. I'm sorry I left that out.
| @@ -432,7 +443,6 @@ public void testTraverseWhenParentDeleted() throws Exception { | |||
| public void testTraverseWhenRootParentDeleted() throws Exception { | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
OK. Let me roll back it.
Thank you very much, I will open a new JIRA to do this. |
|
💔 -1 overall
This message was automatically generated. |
Hi @ayushtkn , the cc warnings seem unrelated to the changes. |
|
I have retriggered the build. Lets wait. If all clean will push this post that |
Thanks @ayushtkn for your help. |
|
💔 -1 overall
This message was automatically generated. |
|
The cc warnings seem related to |
…of paths to be processed by SPS
|
💔 -1 overall
This message was automatically generated. |
|
Hi @ayushtkn , the jenkins log seems compiled failed. But I can compiled successfully locally. And the cc warings seem related to mapreduce-client, but not the changes. Do we need to retrigger the build? Thanks. |
|
Hi @aajisaka , do you know the cause of these cc warnings? This PR does not change the c++ code associated with mapreduce-client. Could you please take a look? Thank you very much. /results-compile-cc-root-jdkPrivateBuild-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07.txt |
|
💔 -1 overall
This message was automatically generated. |
|
Anyway it says If by chance it isn't will revert |
|
Thanks @ayushtkn . |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…of paths to be processed by SPS (apache#4009). Contributed by tomscut. Signed-off-by: Ayush Saxena <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
JIRA: HDFS-16477.
Currently we have no idea how many paths are waiting to be processed when using the SPS feature. We should add metric
PendingSPSPathsfor getting the number of paths to be processed by SPS in NameNode.