HBASE-24898 Use EnvironmentEdge.currentTime() instead of System.currentTimeMillis() in CurrentHourProvider #2272
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
virajjasani
reviewed
Aug 18, 2020
| Set to -1 to disable off-peak.</description> | ||
| </property> | ||
| <property> | ||
| <name>hbase.offpeak.end.hour</name> | ||
| <value>-1</value> | ||
| <description>The end of off-peak hours, expressed as an integer between 0 and 23, inclusive. Set | ||
| <description>The end of off-peak hours, expressed as an integer between 0 and 24, exclusive. Set |
There was a problem hiding this comment.
How is this exclusive for 24 based on return 0 <= hour && hour <= 24; condition?
There was a problem hiding this comment.
@Override public boolean isOffPeakHour(int targetHour) { if (startHour <= endHour) { return startHour <= targetHour && targetHour < endHour; } return targetHour < endHour || startHour <= targetHour; }
See the code, it will be "return 0 <= hour && hour < 24"
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
…ntTimeMillis() in CurrentHourProvider
bsglz
changed the title
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
But I'm not sure if we could control the return value of CurrentHourProvider? In nextTick method, we will not use EnvironmentEdge.currentTime() so it will still return the current time? |
Oh, no, seems you are right, the changing is not enough. |
|
Maybe we can set the time.
|
|
Seems it is ok, will add a unit test for CurrentHourProvider later. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
requested changes
Aug 20, 2020
| return tick.currentHour; | ||
| } | ||
|
|
||
| CurrentHourProvider.tick = tick = nextTick(); | ||
| return tick.currentHour; | ||
| } | ||
|
|
||
| protected static void forceUpdateTickForTest() { |
There was a problem hiding this comment.
I think this can be done by implementing a special EnvironmentEdge? Where we will return a pre defined sequence of timestamp.
There was a problem hiding this comment.
Do you mean like this:
if (EnvironmentEdgeManager.currentTime() <= 23) { return (int)EnvironmentEdgeManager.currentTime(); }
There was a problem hiding this comment.
Then we can inject a special value.
EnvironmentEdgeManager.injectEdge(()->0L); assertEquals(0, CurrentHourProvider.getCurrentHour()); EnvironmentEdgeManager.injectEdge(()->15L); assertEquals(15, CurrentHourProvider.getCurrentHour());
There was a problem hiding this comment.
class MockEnvironmentEdge implements EnvironmentEdge {
private final Queue<Long> times = new ArrayDeque<>();
public void inject(long timestamp) {
times.addLast(timestamp);
}
public long currentTime() {
return times.pollFirst();
}
}
EnvironmentEdgeManager.injectEdge(new MockEnvironmentEdge());
Something like this.
There was a problem hiding this comment.
A bit confusion of why we need a queue?
There was a problem hiding this comment.
You can use any Collections you want. The point here, is that you can inject multiple values at once, and also you can implement more complicated logic in the currentTime method, so you do not need to add a forceUpdateTickForTest method.
There was a problem hiding this comment.
Oh, but i think the test failure above is caused by injectEdge in other test, if EnvironmentEdgeManager.currentTime() < tick.expirationTimeInMillis, then it will return the previous currentHour, it is why i want force update the tick.
There was a problem hiding this comment.
How about the way i mentioned above, a bit magic, but seems bring smallest impact.
if (EnvironmentEdgeManager.currentTime() <= 23) {
return (int)EnvironmentEdgeManager.currentTime();
}
There was a problem hiding this comment.
Then just make the tick field as package private and add a VisibleForTesting annotation? Just change it directly in UT.
There was a problem hiding this comment.
Also the nextTick(), let see the qa result for the recent build first.
|
Seems QA stuck in build#3, can you help to stop it? Thanks. @Apache9 |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Digged more, and i think the failure in fact caused by timezone, let see the new build for verify. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
requested changes
Aug 21, 2020
| * | ||
| * @return Current timezone. | ||
| */ | ||
| default TimeZone currentTimeZone() { |
There was a problem hiding this comment.
A timestamp does not need TimeZone I assume?
| @Test | ||
| public void testWithEnvironmentEdge() { | ||
| // set 1597895561000 with timezone GMT+08:00, represent 2020-08-20 11:52:41, should return 11 | ||
| EnvironmentEdge edgeForHour11 = new EnvironmentEdge() { |
There was a problem hiding this comment.
I would like to do this in a oppsite way, that is, based on the current time zone, we adjust the currentTime, so we will return the same hour.
In general, we should try our best to not add additional logic to normal code only if we want to write a UT.
There was a problem hiding this comment.
Make sense, let me try.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
approved these changes
Aug 21, 2020
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
pushed a commit
that referenced
this pull request
Aug 22, 2020
…ntTimeMillis() in CurrentHourProvider Closes #2272 Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
virajjasani
pushed a commit
that referenced
this pull request
Aug 22, 2020
…ntTimeMillis() in CurrentHourProvider Closes #2272 Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
virajjasani
pushed a commit
that referenced
this pull request
Aug 22, 2020
…ntTimeMillis() in CurrentHourProvider Closes #2272 Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
virajjasani
pushed a commit
that referenced
this pull request
Aug 22, 2020
…ntTimeMillis() in CurrentHourProvider Closes #2272 Co-authored-by: Viraj Jasani <[email protected]> Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
clarax
pushed a commit
to clarax/hbase
that referenced
this pull request
Nov 15, 2020
…ntTimeMillis() in CurrentHourProvider Closes apache#2272 Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
May 24, 2021
…ntTimeMillis() in CurrentHourProvider Closes apache#2272 Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> (cherry picked from commit 9dea961) Change-Id: Ia6ab97f57505a21ba684d56201ffd3226d537e1b
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.
No description provided.