You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/02/22 22:13:04 UTC
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2515: replace some calculation with TimeUnit
ctubbsii commented on a change in pull request #2515:
URL: https://github.com/apache/accumulo/pull/2515#discussion_r812394724
##########
File path: core/src/test/java/org/apache/accumulo/core/conf/ConfigurationTypeHelperTest.java
##########
@@ -80,11 +81,11 @@ public void testGetMemoryAsBytesFailureCases2() {
@Test
public void testGetTimeInMillis() {
- assertEquals(42L * 24 * 60 * 60 * 1000, ConfigurationTypeHelper.getTimeInMillis("42d"));
- assertEquals(42L * 60 * 60 * 1000, ConfigurationTypeHelper.getTimeInMillis("42h"));
- assertEquals(42L * 60 * 1000, ConfigurationTypeHelper.getTimeInMillis("42m"));
- assertEquals(42L * 1000, ConfigurationTypeHelper.getTimeInMillis("42s"));
- assertEquals(42L * 1000, ConfigurationTypeHelper.getTimeInMillis("42"));
+ assertEquals(DAYS.toMillis(42), ConfigurationTypeHelper.getTimeInMillis("42d"));
+ assertEquals(HOURS.toMillis(42), ConfigurationTypeHelper.getTimeInMillis("42h"));
+ assertEquals(MINUTES.toMillis(42), ConfigurationTypeHelper.getTimeInMillis("42m"));
+ assertEquals(SECONDS.toMillis(42), ConfigurationTypeHelper.getTimeInMillis("42s"));
+ assertEquals(42_000L, ConfigurationTypeHelper.getTimeInMillis("42"));
Review comment:
Why not?
```suggestion
assertEquals(SECONDS.toMillis(42), ConfigurationTypeHelper.getTimeInMillis("42"));
```
##########
File path: core/src/test/java/org/apache/accumulo/core/rpc/TTimeoutTransportTest.java
##########
@@ -75,7 +76,7 @@ public void testFailedSocketOpenIsClosed() throws IOException {
@Test
public void testFailedInputStreamClosesSocket() throws IOException {
- long timeout = 2 * 60_000; // 2 mins
+ long timeout = MINUTES.toMillis(2); // 2 mins
Review comment:
```suggestion
long timeout = MINUTES.toMillis(2);
```
##########
File path: core/src/main/java/org/apache/accumulo/core/classloader/DefaultContextClassLoaderFactory.java
##########
@@ -69,7 +71,7 @@ private static void startCleanupThread(final AccumuloConfiguration conf,
.collect(Collectors.toSet());
LOG.trace("{}-cleanup thread, contexts in use: {}", className, contextsInUse);
AccumuloVFSClassLoader.removeUnusedContexts(contextsInUse);
- }), 60_000, 60_000, TimeUnit.MILLISECONDS);
+ }), MINUTES.toMillis(1), MINUTES.toMillis(1), TimeUnit.MILLISECONDS);
Review comment:
You could static import the MILLISECONDS as well, to be consistent. Or, in this case, you could do:
```suggestion
}), 1, 1, MINUTES);
```
##########
File path: test/src/main/java/org/apache/accumulo/test/functional/KerberosIT.java
##########
@@ -616,13 +617,13 @@ public void testDelegationTokenWithReducedLifetime() throws Throwable {
assertEquals(rootUser.getPrincipal(), client.whoami());
return client.securityOperations().getDelegationToken(
- new DelegationTokenConfig().setTokenLifetime(5, TimeUnit.MINUTES));
+ new DelegationTokenConfig().setTokenLifetime(5, MINUTES));
}
});
AuthenticationTokenIdentifier identifier = ((DelegationTokenImpl) dt).getIdentifier();
assertTrue("Expected identifier to expire in no more than 5 minutes: " + identifier,
- identifier.getExpirationDate() - identifier.getIssueDate() <= (5 * 60_000));
+ identifier.getExpirationDate() - identifier.getIssueDate() <= (MINUTES.toMillis(5)));
Review comment:
After the conversion, this has some extra parens that could be removed:
```suggestion
identifier.getExpirationDate() - identifier.getIssueDate() <= MINUTES.toMillis(5));
```
##########
File path: core/src/test/java/org/apache/accumulo/core/rpc/TTimeoutTransportTest.java
##########
@@ -105,7 +106,7 @@ public void testFailedInputStreamClosesSocket() throws IOException {
@Test
public void testFailedOutputStreamClosesSocket() throws IOException {
- long timeout = 2 * 60_000; // 2 mins
+ long timeout = MINUTES.toMillis(2); // 2 mins
Review comment:
```suggestion
long timeout = MINUTES.toMillis(2);
```
##########
File path: test/src/main/java/org/apache/accumulo/test/functional/FateConcurrencyIT.java
##########
@@ -135,7 +136,7 @@ public void changeTableStateTest() throws Exception {
OnlineOpTiming timing1 = task.get();
log.trace("Online 1 in {} ms",
- TimeUnit.MILLISECONDS.convert(timing1.runningTime(), TimeUnit.NANOSECONDS));
+ MILLISECONDS.convert(timing1.runningTime(), NANOSECONDS));
Review comment:
`.convert()` is a very confusing, because it's not obvious which direction it is converting. The following would be better for these:
```suggestion
NANOSECONDS.toMillis(timing1.runningTime()));
```
##########
File path: server/base/src/test/java/org/apache/accumulo/server/security/delegation/ZooAuthenticationKeyWatcherTest.java
##########
@@ -66,7 +68,7 @@ public static void setupKeyGenerator() throws Exception {
private ZooReader zk;
private InstanceId instanceId;
private String baseNode;
- private long tokenLifetime = 7 * 24 * 60 * 60_000; // 7days
+ private long tokenLifetime = DAYS.toMillis(7); // 7days
Review comment:
```suggestion
private long tokenLifetime = DAYS.toMillis(7);
```
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -434,7 +435,7 @@ private void monitorSwappiness() {
} catch (Exception t) {
log.error("", t);
}
- }, 1000, 10 * 60_000, TimeUnit.MILLISECONDS);
+ }, 1000, MINUTES.toMillis(10), TimeUnit.MILLISECONDS);
Review comment:
```suggestion
}, SECONDS.toMillis(1), MINUTES.toMillis(10), TimeUnit.MILLISECONDS);
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org