[GitHub] storm pull request #2787: STORM-3169: Correctly convert configured minutes t...
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2787 ---
[GitHub] storm pull request #2787: STORM-3169: Correctly convert configured minutes t...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2787#discussion_r207329210
--- Diff:
storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/LogCleanerTest.java
---
@@ -87,8 +88,9 @@ public void testMkFileFilterForLogCleanup() throws
IOException {
final long nowMillis = Time.currentTimeMillis();
final long cutoffMillis =
logCleaner.cleanupCutoffAgeMillis(nowMillis);
-final long oldMtimeMillis = cutoffMillis - 500;
-final long newMtimeMillis = cutoffMillis + 500;
+final long interval = TimeUnit.MINUTES.toMillis(500);
--- End diff --
Ah I missed that. My bad. You're right.
---
[GitHub] storm pull request #2787: STORM-3169: Correctly convert configured minutes t...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2787#discussion_r207328980
--- Diff:
storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/LogCleanerTest.java
---
@@ -87,8 +88,9 @@ public void testMkFileFilterForLogCleanup() throws
IOException {
final long nowMillis = Time.currentTimeMillis();
final long cutoffMillis =
logCleaner.cleanupCutoffAgeMillis(nowMillis);
-final long oldMtimeMillis = cutoffMillis - 500;
-final long newMtimeMillis = cutoffMillis + 500;
+final long interval = TimeUnit.MINUTES.toMillis(500);
--- End diff --
Just to try to be a little more clear, the test needs a timestamp before
the cutoff, and a timestamp after the cutoff. The cutoff is `now - 60 minutes`.
`now - 60 minutes - 500 ms` is before the cutoff. I don't understand why it
instead needs to be `now - 60 minutes - 500 minutes`?
---
[GitHub] storm pull request #2787: STORM-3169: Correctly convert configured minutes t...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2787#discussion_r207328445
--- Diff:
storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/LogCleanerTest.java
---
@@ -87,8 +88,9 @@ public void testMkFileFilterForLogCleanup() throws
IOException {
final long nowMillis = Time.currentTimeMillis();
final long cutoffMillis =
logCleaner.cleanupCutoffAgeMillis(nowMillis);
-final long oldMtimeMillis = cutoffMillis - 500;
-final long newMtimeMillis = cutoffMillis + 500;
+final long interval = TimeUnit.MINUTES.toMillis(500);
--- End diff --
Not sure I follow. The cutoffMillis is calculated based on the 60 minutes,
so would be `now - 60 minutes`. It should be the same cutoffMillis used by the
log cleaner. 500 milliseconds before the cutoff is before the cutoff, right?
---
[GitHub] storm pull request #2787: STORM-3169: Correctly convert configured minutes t...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2787#discussion_r207326393
--- Diff:
storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/LogCleanerTest.java
---
@@ -87,8 +88,9 @@ public void testMkFileFilterForLogCleanup() throws
IOException {
final long nowMillis = Time.currentTimeMillis();
final long cutoffMillis =
logCleaner.cleanupCutoffAgeMillis(nowMillis);
-final long oldMtimeMillis = cutoffMillis - 500;
-final long newMtimeMillis = cutoffMillis + 500;
+final long interval = TimeUnit.MINUTES.toMillis(500);
--- End diff --
LOGVIEWER_CLEANUP_AGE_MINS is set to 60 (minutes). In this case the cut off
age will be set to 60 minutes ago. 500 milliseconds will still be after cut off.
---
[GitHub] storm pull request #2787: STORM-3169: Correctly convert configured minutes t...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2787#discussion_r207325422
--- Diff:
storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/LogCleanerTest.java
---
@@ -87,8 +88,9 @@ public void testMkFileFilterForLogCleanup() throws
IOException {
final long nowMillis = Time.currentTimeMillis();
final long cutoffMillis =
logCleaner.cleanupCutoffAgeMillis(nowMillis);
-final long oldMtimeMillis = cutoffMillis - 500;
-final long newMtimeMillis = cutoffMillis + 500;
+final long interval = TimeUnit.MINUTES.toMillis(500);
--- End diff --
I don't think this change is necessary. The test is mocking some files for
right before and right after the cutoff. Why do we need to make it 500 minutes
before/after the cutoff instead of 500 milliseconds?
---
[GitHub] storm pull request #2787: STORM-3169: Correctly convert configured minutes t...
GitHub user zd-project opened a pull request: https://github.com/apache/storm/pull/2787 STORM-3169: Correctly convert configured minutes to milliseconds See https://issues.apache.org/jira/browse/STORM-3169 for details You can merge this pull request into a Git repository by running: $ git pull https://github.com/zd-project/storm STORM-3169 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2787.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2787 commit fe0e5f4ddbb2260ad442320bceb99bb28a96fb33 Author: Zhengdai Hu Date: 2018-08-01T20:59:17Z STORM-3169: Correctly convert configured minutes to milliseconds ---
