bbeaudreault commented on PR #4312:
URL: https://github.com/apache/hbase/pull/4312#issuecomment-1096671117
I'm having trouble getting this PR diff to load in github, but I took a look
locally. Overall this looks good, from what I was able to scan over. I did
notice a few small things. **None of these are blockers**.
-----
In FanOutOneBlockAsyncDFSOutputHelper.java, the below is a large departure
from current convention. I think this format is especially useful with some
blocks (especially method chaining), but one awkward thing here is that the
first argument is still on the first line. All of the other arguments are on
their own line. Personally, if we're going to make every argument its own line
like this, i'd probably rather see the first argument on its own line too. It
just makes it easier to scan arguments visually since they are all lined up.
So my proposal: move "create" to second line, or condense it so that it's
not so many lines. One or the other.
```
+ Method createMethod = ClientProtocol.class.getMethod("create",
+ String.class,
+ FsPermission.class,
+ String.class,
+ EnumSetWritable.class,
+ boolean.class,
+ short.class,
+ long.class,
+ CryptoProtocolVersion[].class,
+ String.class,
+ String.class);
```
In the same file, similar thing here but with method chaining. Personally
I'd prefer that we move `.group(eventLoopGroup)` to next line. It just makes it
a bit easier to scan all the chained methods.
```
+ new Bootstrap().group(eventLoopGroup)
+ .channel(channelClass)
+ .option(CONNECT_TIMEOUT_MILLIS, timeoutMs)
```
-----
In TestRecoverLeaseFSUtils.java, this math + comparison ended up splitting
somewhat awkwardly.
```
+ assertTrue((EnvironmentEdgeManager.currentTime() - startTime) > (3
+ * HTU.getConfiguration().getInt("hbase.lease.recovery.dfs.timeout",
61000)));
```
Proposed alternative, if possible:
```
+ assertTrue(
+ (EnvironmentEdgeManager.currentTime() - startTime)
+ >
+ (3 *
HTU.getConfiguration().getInt("hbase.lease.recovery.dfs.timeout", 61000)));
```
Or put the less than sign on one of those 2 lines. But the main thing trying
to avoid splitting up the parenthesis.
----
The ternary operator is awkwardly split. There are a few examples of this,
but one is from In
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManifest.java:
```
+ BackupType type = im.getBackupType() == BackupProtos.BackupType.FULL
? BackupType.FULL
+ : BackupType.INCREMENTAL;
```
I think this format is more readable (from BackupCommands.java):
```
+ int bandwidth = cmdline.hasOption(OPTION_BANDWIDTH)
+ ? Integer.parseInt(cmdline.getOptionValue(OPTION_BANDWIDTH))
+ : -1;
```
I imagine this is currently being driven by line length, but I think it's
nicer to be consistent for ternary.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
