kfaraz commented on code in PR #16563:
URL: https://github.com/apache/druid/pull/16563#discussion_r1633131036
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -2449,21 +2445,24 @@ private Map<String, String>
getAppendSegmentsCommittedDuringTask(
return segmentIdToLockVersion;
}
- private Set<String> segmentExistsBatch(final Handle handle, final
Set<DataSegment> segments)
+ private Set<String> findExistingSegmentIds(final Handle handle, final
Set<DataSegment> segments)
{
- Set<String> existedSegments = new HashSet<>();
+ final Set<String> existingSegmentIds = new HashSet<>();
- List<List<DataSegment>> segmentsLists = Lists.partition(new
ArrayList<>(segments), MAX_NUM_SEGMENTS_TO_ANNOUNCE_AT_ONCE);
+ final List<List<DataSegment>> segmentsLists = Lists.partition(
+ new ArrayList<>(segments),
+ SEGMENT_INSERT_BATCH_SIZE
+ );
Review Comment:
The soft limit for better readability is actually 80 chars.
120 chars is the hard limit which we should not exceed.
---
Obviously, all of this is subjective and depends on personal coding style.
But generally, I too avoid breaking lines in method/constructor calls if
- they can be fit "comfortably" in a single line
- they are not difficult to follow
The end goal is to have easily readable code.
So I would prefer
```java
performAction(arg1, arg2, arg3, arg4, arg5);
```
over
```java
performAction(
arg1,
arg2,
arg3,
arg4,
arg5
);
```
but NOT
```java
performAction(computeFirstArg(), object.getSecondArg(), arg3, arg4,
thirdArgIfNonNull());
```
over
```java
performAction(
computeFirstArg(),
object.getSecondArg(),
arg3,
arg4,
thirdArgIfNonNull()
);
```
For the case at hand, we have two options:
Option 1:
```java
final List<List<DataSegment>> segmentsLists = Lists.partition(
new ArrayList<>(segments),
SEGMENT_INSERT_BATCH_SIZE
);
```
Option 2:
```java
final List<List<DataSegment>> segmentsLists = Lists.partition(new
ArrayList<>(segments), SEGMENT_INSERT_BATCH_SIZE);
```
I feel Option 1 here reads better for 2 reasons:
- Option 2 is definitely too long, it's right at the 120 char limit.
- Option 2 is a little more complex cognitively, it has too much stuff
happening in a single line.
Hope you find that helpful. 🙂
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
