You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/09/28 08:33:53 UTC
[GitHub] [ozone] JacksonYao287 opened a new pull request #2685: HDDS-5734. add a command to trigger datanode executing du immediately
JacksonYao287 opened a new pull request #2685:
URL: https://github.com/apache/ozone/pull/2685
## What changes were proposed in this pull request?
add a command to trigger datanode executing du immediately at datanode side
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5734
## How was this patch tested?
unit test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2685: HDDS-5734. add a command to trigger datanode executing du immediately
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2685:
URL: https://github.com/apache/ozone/pull/2685#discussion_r717537735
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java
##########
@@ -45,31 +38,18 @@
private static final Logger LOG =
LoggerFactory.getLogger(CachingSpaceUsageSource.class);
- private final ScheduledExecutorService executor;
private final AtomicLong cachedValue = new AtomicLong();
- private final Duration refresh;
+ private final Duration refreshInterVal;
private final SpaceUsageSource source;
private final SpaceUsagePersistence persistence;
private boolean running;
- private ScheduledFuture<?> scheduledFuture;
-
- public CachingSpaceUsageSource(SpaceUsageCheckParams params) {
- this(params, createExecutor(params));
- }
@VisibleForTesting
- CachingSpaceUsageSource(SpaceUsageCheckParams params,
- ScheduledExecutorService executor) {
Review comment:
Being able to provide `executor` is helpful for testing. I think switching to a thread that's always started, even for unit tests, is a step back. I think immediate refresh can be achieved by scheduling a new refresh task without delay (keeping track of last refresh time might be useful as well).
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java
##########
@@ -45,31 +38,18 @@
private static final Logger LOG =
LoggerFactory.getLogger(CachingSpaceUsageSource.class);
- private final ScheduledExecutorService executor;
private final AtomicLong cachedValue = new AtomicLong();
- private final Duration refresh;
+ private final Duration refreshInterVal;
private final SpaceUsageSource source;
private final SpaceUsagePersistence persistence;
private boolean running;
- private ScheduledFuture<?> scheduledFuture;
-
- public CachingSpaceUsageSource(SpaceUsageCheckParams params) {
- this(params, createExecutor(params));
- }
@VisibleForTesting
Review comment:
No longer `@VisibleForTesting`, since this is the constructor used by production code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2685: HDDS-5734. add a command to trigger datanode executing du immediately
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2685:
URL: https://github.com/apache/ozone/pull/2685#discussion_r717537735
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java
##########
@@ -45,31 +38,18 @@
private static final Logger LOG =
LoggerFactory.getLogger(CachingSpaceUsageSource.class);
- private final ScheduledExecutorService executor;
private final AtomicLong cachedValue = new AtomicLong();
- private final Duration refresh;
+ private final Duration refreshInterVal;
private final SpaceUsageSource source;
private final SpaceUsagePersistence persistence;
private boolean running;
- private ScheduledFuture<?> scheduledFuture;
-
- public CachingSpaceUsageSource(SpaceUsageCheckParams params) {
- this(params, createExecutor(params));
- }
@VisibleForTesting
- CachingSpaceUsageSource(SpaceUsageCheckParams params,
- ScheduledExecutorService executor) {
Review comment:
Being able to provide `executor` is helpful for testing. I think switching to a thread that's always started, even for unit tests, is a step back. I think immediate refresh can be achieved by scheduling a new refresh task without delay (keeping track of last refresh time might be useful as well).
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java
##########
@@ -45,31 +38,18 @@
private static final Logger LOG =
LoggerFactory.getLogger(CachingSpaceUsageSource.class);
- private final ScheduledExecutorService executor;
private final AtomicLong cachedValue = new AtomicLong();
- private final Duration refresh;
+ private final Duration refreshInterVal;
private final SpaceUsageSource source;
private final SpaceUsagePersistence persistence;
private boolean running;
- private ScheduledFuture<?> scheduledFuture;
-
- public CachingSpaceUsageSource(SpaceUsageCheckParams params) {
- this(params, createExecutor(params));
- }
@VisibleForTesting
Review comment:
No longer `@VisibleForTesting`, since this is the constructor used by production code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #2685: HDDS-5734. add a command to trigger datanode executing du immediately
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2685:
URL: https://github.com/apache/ozone/pull/2685#issuecomment-938618648
> I think we should also make sure that DU is not triggered aggressively.
@lokeshj1703 thanks for the review, i have address this issue , please take a look!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on pull request #2685: HDDS-5734. add a command to trigger datanode executing du immediately
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #2685:
URL: https://github.com/apache/ozone/pull/2685#issuecomment-946636743
@JacksonYao287 Thanks for the contribution! @adoroszlai Thanks for the reviews! I have committed the PR to master branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2685: HDDS-5734. add a command to trigger datanode executing du immediately
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2685:
URL: https://github.com/apache/ozone/pull/2685#issuecomment-938618648
> I think we should also make sure that DU is not triggered aggressively.
@lokeshj1703 thanks for the review, i have address this issue , please take a look!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2685: HDDS-5734. add a command to trigger datanode executing du immediately
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2685:
URL: https://github.com/apache/ozone/pull/2685#discussion_r725896656
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java
##########
@@ -114,16 +115,26 @@ public void shutdown() {
}
}
+ public void refreshNow() {
+ //refresh immediately
+ executor.schedule(this::refresh, 0, MILLISECONDS);
+ }
+
private void loadInitialValue() {
final OptionalLong initialValue = persistence.load();
initialValue.ifPresent(cachedValue::set);
}
private void refresh() {
- try {
- cachedValue.set(source.getUsedSpace());
- } catch (RuntimeException e) {
- LOG.warn("Error refreshing space usage for {}", source, e);
+ //only one `refresh` can be running at a certain moment
+ if(isRefreshRunning.compareAndSet(false, true)) {
Review comment:
Note: this guards against concurrent execution, but not against too frequent serial execution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2685: HDDS-5734. add a command to trigger datanode executing du immediately
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2685:
URL: https://github.com/apache/ozone/pull/2685#discussion_r719567266
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java
##########
@@ -45,31 +38,18 @@
private static final Logger LOG =
LoggerFactory.getLogger(CachingSpaceUsageSource.class);
- private final ScheduledExecutorService executor;
private final AtomicLong cachedValue = new AtomicLong();
- private final Duration refresh;
+ private final Duration refreshInterVal;
private final SpaceUsageSource source;
private final SpaceUsagePersistence persistence;
private boolean running;
- private ScheduledFuture<?> scheduledFuture;
-
- public CachingSpaceUsageSource(SpaceUsageCheckParams params) {
- this(params, createExecutor(params));
- }
@VisibleForTesting
- CachingSpaceUsageSource(SpaceUsageCheckParams params,
- ScheduledExecutorService executor) {
Review comment:
good idea, thanks, i will try this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #2685: HDDS-5734. add a command to trigger datanode executing du immediately
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2685:
URL: https://github.com/apache/ozone/pull/2685#issuecomment-931472273
thanks @adoroszlai for the review! i will update this patch in a new commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2685: HDDS-5734. add a command to trigger datanode executing du immediately
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2685:
URL: https://github.com/apache/ozone/pull/2685#discussion_r724971720
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java
##########
@@ -76,15 +76,18 @@ private long getRemainingReserved() {
return Math.max(reservedInBytes - getOtherUsed(), 0L);
}
- public synchronized void start() {
Review comment:
yea，sorry for this. i used `wait` in earlier commit of this PR, and add a `synchronized` with it, so i removed the one here and forget to add it back in the latest commit. i will add it back in a new commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2685: HDDS-5734. add a command to trigger datanode executing du immediately
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2685:
URL: https://github.com/apache/ozone/pull/2685#discussion_r721971787
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java
##########
@@ -76,15 +76,18 @@ private long getRemainingReserved() {
return Math.max(reservedInBytes - getOtherUsed(), 0L);
}
- public synchronized void start() {
Review comment:
I think we should keep `start` and `shutdown` synchronized. They may be called in various threads. Any specific reason to remove it now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 merged pull request #2685: HDDS-5734. add a command to trigger datanode executing du immediately
Posted by GitBox <gi...@apache.org>.
lokeshj1703 merged pull request #2685:
URL: https://github.com/apache/ozone/pull/2685
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #2685: HDDS-5734. add a command to trigger datanode executing du immediately
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2685:
URL: https://github.com/apache/ozone/pull/2685#issuecomment-946557266
@lokeshj1703 would you like to take another look, or can we merge it now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2685: HDDS-5734. add a command to trigger datanode executing du immediately
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2685:
URL: https://github.com/apache/ozone/pull/2685#discussion_r719566823
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java
##########
@@ -45,31 +38,18 @@
private static final Logger LOG =
LoggerFactory.getLogger(CachingSpaceUsageSource.class);
- private final ScheduledExecutorService executor;
private final AtomicLong cachedValue = new AtomicLong();
- private final Duration refresh;
+ private final Duration refreshInterVal;
private final SpaceUsageSource source;
private final SpaceUsagePersistence persistence;
private boolean running;
- private ScheduledFuture<?> scheduledFuture;
-
- public CachingSpaceUsageSource(SpaceUsageCheckParams params) {
- this(params, createExecutor(params));
- }
@VisibleForTesting
Review comment:
sure, will fix this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org