[GitHub] [hbase] apurtell commented on pull request #3302: HBASE-25911 Replace calls to System.currentTimeMillis with EnvironmentEdgeManager.currentTime
apurtell commented on pull request #3302: URL: https://github.com/apache/hbase/pull/3302#issuecomment-850751391 I'm still waiting for tests to finish. Things look fine so far, want to be sure. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3302: HBASE-25911 Replace calls to System.currentTimeMillis with EnvironmentEdgeManager.currentTime
apurtell commented on pull request #3302: URL: https://github.com/apache/hbase/pull/3302#issuecomment-850572028 Thanks for the approvals. I'm going to rebase, run tests locally, and merge if things look good. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3302: HBASE-25911 Replace calls to System.currentTimeMillis with EnvironmentEdgeManager.currentTime
apurtell commented on pull request #3302: URL: https://github.com/apache/hbase/pull/3302#issuecomment-850571678 > It looks like, unless we are too specific about elapsed time that we want only nanosecond level elapsed time, we should prefer System#currentTimeMillis for the perf benefit. @virajjasani I think this is something best discussed in a different context. Should we file a JIRA to discuss what to do, if anything, about nanotime? Also, please read [Nanotrusting The Nanotime](https://shipilev.net/blog/2014/nanotrusting-nanotime/). -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3302: HBASE-25911 Replace calls to System.currentTimeMillis with EnvironmentEdgeManager.currentTime
apurtell commented on pull request #3302: URL: https://github.com/apache/hbase/pull/3302#issuecomment-850569022 > But sometimes we use System.currentTimeMillis just for calculating elapsed time, not for getting system clock, I'm not sure if this is also our goal here, to also replace these usages. @Apache9 This is not the goal. > We introduced EnvironmentEdgeManager a long time ago as a way to inject alternate clocks (gettimeofday() aka System.currentTimeMillis()) for unit tests. In order for this to be effective, all callers that would otherwise use System.currentTimeMillis() must call EnvironmentEdgeManager.currentTime() instead, except obviously the implementors of EnvironmentEdge. > > It's common for contributors to be unaware of this practice and reviewers might not catch it. The goal is to fix the code is out of practice with EnvironmentEdgeManager#currentTime. We can also do something about nanotime, but under another JIRA, please. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3302: HBASE-25911 Replace calls to System.currentTimeMillis with EnvironmentEdgeManager.currentTime
apurtell commented on pull request #3302: URL: https://github.com/apache/hbase/pull/3302#issuecomment-849872794 > What about System.nanoTime? @Apache9 System.nanotime is not used to get the system clock. I don't mind bringing it into EnvironmentEdge but I don't care about it here. It is out of scope for all of this work. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3302: HBASE-25911 Replace calls to System.currentTimeMillis with EnvironmentEdgeManager.currentTime
apurtell commented on pull request #3302: URL: https://github.com/apache/hbase/pull/3302#issuecomment-849871673 TestWALEntryStream failure is not related. It is because of HBASE-25924, which I have reopened. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3302: HBASE-25911 Replace calls to System.currentTimeMillis with EnvironmentEdgeManager.currentTime
apurtell commented on pull request #3302: URL: https://github.com/apache/hbase/pull/3302#issuecomment-849192021 Update fixes a checkstyle nit in SimpleServerRpcConnection. The rest of them aren't mine. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3302: HBASE-25911 Replace calls to System.currentTimeMillis with EnvironmentEdgeManager.currentTime
apurtell commented on pull request #3302: URL: https://github.com/apache/hbase/pull/3302#issuecomment-849094550 Rebase and fix TestRegionServerReportForDuty#testReportForDutyWithEnvironmentEdge. This test injected ManualEnvironmentEdge and never incremented the time. It doesn't matter what the intent of the test was, this was never valid. The only reason it passed was EnvironmentEdge was not used in all places to get the time, as it should have been. Inject IncrementingEnvironmentEdge instead, now the test passes. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3302: HBASE-25911 Replace calls to System.currentTimeMillis with EnvironmentEdgeManager.currentTime
apurtell commented on pull request #3302: URL: https://github.com/apache/hbase/pull/3302#issuecomment-848374865 Update fixes TestSimpleRpcScheduler and TestBackupDelete and checkstyle and javadoc nits. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3302: HBASE-25911 Replace calls to System.currentTimeMillis with EnvironmentEdgeManager.currentTime
apurtell commented on pull request #3302: URL: https://github.com/apache/hbase/pull/3302#issuecomment-848192638 Two test failures were relevant. Updating patch soon. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
