HADOOP-19280. [ABFS] Initialize client timer only if metric collection is enabled #7061
Conversation
This file contains hidden or 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
|
@anujmodi2021 @anmolanmol1234 Please review the PR. |
added 2 commits
September 23, 2024 00:23
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
anmolanmol1234
suggested changes
Sep 23, 2024
| boolean isTimerThreadPresent = isThreadRunning("abfs-timer-client"); | ||
|
|
||
| // Check if a thread with the name "abfs-timer-client" exists | ||
| assertFalse("Unexpected thread 'abfs-timer-client' found", isTimerThreadPresent); |
There was a problem hiding this comment.
Use assertions.assertThat
...tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/ITestAbfsClient.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
…stAbfsClient class
HNS-OAuth[ERROR] testFileSystemCapabilities(org.apache.hadoop.fs.azurebfs.ITestFileSystemInitialization) Time elapsed: 0.402 s <<< FAILURE! [ERROR] testBackoffRetryMetrics(org.apache.hadoop.fs.azurebfs.services.TestAbfsRestOperation) Time elapsed: 1.597 s <<< ERROR! [ERROR] Tests run: 157, Failures: 0, Errors: 1, Skipped: 2 Test failures are known, and some are fixed in the below PRs. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
Outdated
Show resolved
Hide resolved
...-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
@mukund-thakur @steveloughran @saikatroy038 Could you please review the PR? |
anujmodi2021
approved these changes
Sep 26, 2024
There was a problem hiding this comment.
+1 LGTM
Thanks for taking my comments @bhattmanish98
The tests that you have are indeed the ones that are known and fixed/being fixed
Except the CheckAccess tests. Looks like they are failing due to some miss configuration on your end.
anmolanmol1234
suggested changes
Sep 26, 2024
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
Outdated
Show resolved
Hide resolved
anmolanmol1234
approved these changes
Sep 26, 2024
...-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java
Outdated
Show resolved
Hide resolved
...-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
Sep 27, 2024
...-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java
Show resolved
Hide resolved
| import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_METRIC_FORMAT; | ||
| import static org.apache.hadoop.fs.azurebfs.services.AbfsClient.ABFS_CLIENT_TIMER_THREAD_NAME; | ||
|
|
||
| public class TestAbfsClient { |
There was a problem hiding this comment.
Add a little Java explaining what it is trying to test.
| private static final long SLEEP_DURATION_MS = 500; | ||
|
|
||
| @Test | ||
| public void testTimerNotInitialize() throws Exception { |
There was a problem hiding this comment.
- Add a comment stating that this verifies that's a timer is only created when metrics are being collected.
- Change the title of the method to something like
testTimerNotInitializedWithoutMetricCollection
| } | ||
|
|
||
| @Test | ||
| public void testTimerInitialize() throws Exception { |
There was a problem hiding this comment.
- Change the title of the method to something like
testTimerIsInitializedWithMetricCollection
...-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
changed the title
|
Manish, thank you! Please can you do a backport PR for branch-3.4; this is simply for testing rather than any more code reviews. |
@steveloughran Applogies for the delay. I have raised a PR (#7307) to backport these changes to branch-3.4. Thanks |
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.
Description of PR
JIRA: https://issues.apache.org/jira/browse/HADOOP-19280
Current Flow: In the current flow, we are initializing the timer of the abfs-timer-client outside the metric collection enable check. As a result, for each file system, when the AbfsClient object is initialized, it spawns a thread to evaluate the time of the ABFS client. Since we are purging/closing the timer inside the metric collection check, these threads are not being closed, causing them to persist in a long-lived state.
Changes Made: This PR contains the changes related to moving the initialization of the timer inside the metric collection check. Also includes check on metric collection if we are accessing the timer variable to avoid the null pointer exception.
Created two test suites to check the behaviour of timer variable and thread spawn in case of metric collection enabled and disabled.