-
Notifications
You must be signed in to change notification settings - Fork 588
HDDS-12852. Implement a sliding window counter utility #8498
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
Conversation
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ptlrs for the patch.
I suggest removing the concept of failure from SlidingWindow, and let it simply track a limited number of event timestamps.
Rename members accordingly:
failureTolerance -> maxSizefailureTimestamps -> timestampsisFailed() -> isEmpty()(inverting meaning)removeExpiredFailures() -> removeExpired()getFailureCount() -> size()(taken from follow-up PR)
Code tracking volume failures using SlidingWindow should add() only for failed check results, instead of letting the SlidingWindow decide based on an argument.
...iner-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/SlidingWindow.java
Outdated
Show resolved
Hide resolved
...iner-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/SlidingWindow.java
Outdated
Show resolved
Hide resolved
...iner-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/SlidingWindow.java
Outdated
Show resolved
Hide resolved
...iner-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/SlidingWindow.java
Outdated
Show resolved
Hide resolved
Tejaskriya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @ptlrs, Please find my comment below
...iner-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/SlidingWindow.java
Outdated
Show resolved
Hide resolved
|
Hi @adoroszlai @Tejaskriya, the comments have been addressed. Could you please review this PR again. |
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ptlrs for updating the patch. Implementation looks good, I think the test can be improved a bit.
...-service/src/test/java/org/apache/hadoop/ozone/container/common/utils/TestSlidingWindow.java
Outdated
Show resolved
Hide resolved
...-service/src/test/java/org/apache/hadoop/ozone/container/common/utils/TestSlidingWindow.java
Outdated
Show resolved
Hide resolved
...-service/src/test/java/org/apache/hadoop/ozone/container/common/utils/TestSlidingWindow.java
Outdated
Show resolved
Hide resolved
...iner-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/SlidingWindow.java
Outdated
Show resolved
Hide resolved
|
Thanks for the review @adoroszlai. I have updated the tests. |
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ptlrs for updating the patch.
Tejaskriya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch @ptlrs , LGTM
errose28
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @ptlrs. I have some comments in addition to what has already been left.
...iner-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/SlidingWindow.java
Outdated
Show resolved
Hide resolved
...iner-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/SlidingWindow.java
Outdated
Show resolved
Hide resolved
...iner-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/SlidingWindow.java
Outdated
Show resolved
Hide resolved
...iner-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/SlidingWindow.java
Outdated
Show resolved
Hide resolved
...-service/src/test/java/org/apache/hadoop/ozone/container/common/utils/TestSlidingWindow.java
Outdated
Show resolved
Hide resolved
...-service/src/test/java/org/apache/hadoop/ozone/container/common/utils/TestSlidingWindow.java
Show resolved
Hide resolved
...iner-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/SlidingWindow.java
Show resolved
Hide resolved
...iner-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/SlidingWindow.java
Outdated
Show resolved
Hide resolved
|
@errose28 the PR has been updated with the changes. The |
errose28
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one comment on functionality left then I think this is ready to merge.
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/SlidingWindow.java
Show resolved
Hide resolved
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/SlidingWindow.java
Show resolved
Hide resolved
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/TestSlidingWindow.java
Outdated
Show resolved
Hide resolved
…alization in SlidingWindow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @ptlrs. Next we can start using this to simplify the volume scanners' failure criteria. Should be good to merge once CI finishes.
|
Thanks for the extensive reviews @adoroszlai @errose28 @Tejaskriya. |
|
Thanks for the patch @ptlrs and the reviews @adoroszlai @errose28 |
* master: (730 commits) HDDS-13083. Handle cases where block deletion generates tree file before scanner (apache#8565) HDDS-12982. Reduce log level for snapshot validation failure (apache#8851) HDDS-13396. Documentation: Improve the top-level overview page for new users. (apache#8753) HDDS-13176. containerIds table value format change to proto from string (apache#8589) HDDS-13449. Incorrect Interrupt Handling for DirectoryDeletingService and KeyDeletingService (apache#8817) HDDS-2453. Add Freon tests for S3 MPU Keys (apache#8803) HDDS-13237. Container data checksum should contain block IDs. (apache#8773) HDDS-13489. Fix SCMBlockdeleting unnecessary iteration in corner case. (apache#8847) HDDS-13464. Make ozone.snapshot.filtering.service.interval reconfigurable (apache#8825) HDDS-13473. Amend validation for OZONE_OM_SNAPSHOT_DB_MAX_OPEN_FILES (apache#8829) HDDS-13435. Add an OzoneManagerAuthorizer interface (apache#8840) HDDS-8565. Recon memory leak in NSSummary (apache#8823). HDDS-12852. Implement a sliding window counter utility (apache#8498) HDDS-12000. Add unit test for RatisContainerSafeModeRule and ECContainerSafeModeRule (apache#8801) HDDS-13092. Container scanner should trigger volume scan when marking a container unhealthy (apache#8603) HDDS-13070. OM Follower changes to create and place sst files from hardlink file. (apache#8761) HDDS-13482. Mark testWriteStateMachineDataIdempotencyWithClosedContainer as flaky HDDS-13481. Fix success latency metric in SCM panels of deletion grafana dashboard (apache#8835) HDDS-13468. Update default value of ozone.scm.ha.dbtransactionbuffer.flush.interval. (apache#8834) HDDS-13410. Control block deletion for each DN from SCM. (apache#8767) ... hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReplicaInfo.java hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/ReconcileSubcommand.java hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/hdds/scm/cli/container/TestReconcileSubcommand.java
What changes were proposed in this pull request?
Please describe your PR in detail:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12852
How was this patch tested?
Added new unit tests
CI: https://github.com/ptlrs/ozone/actions/runs/16235740233