[GitHub] gianm commented on a change in pull request #6068: Prohibit Lists.newArrayList() with a single argument
gianm commented on a change in pull request #6068: Prohibit Lists.newArrayList() with a single argument URL: https://github.com/apache/incubator-druid/pull/6068#discussion_r206212950 ## File path: processing/src/main/java/io/druid/collections/spatial/Node.java ## @@ -56,7 +57,7 @@ public Node(float[] minCoordinates, float[] maxCoordinates, boolean isLeaf, Bitm public Node( float[] minCoordinates, float[] maxCoordinates, - List children, + @Nullable Node child, Review comment: On YAGNI vs BDUF: IMO, YAGNI is better for internal APIs (which this is) so it is appropriate to get rid of the list. BDUF has some value for external APIs that we can't change later without disrupting users. It can pay to try to think about how usage may evolve over time, before the APIs are introduced. So my 2ยข is that as an internal API, it's better for this one to offer no more than we need today. This is an automated message from the Apache Git Service. To respond to the message, please log on 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 With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] gianm commented on a change in pull request #6068: Prohibit Lists.newArrayList() with a single argument
gianm commented on a change in pull request #6068: Prohibit
Lists.newArrayList() with a single argument
URL: https://github.com/apache/incubator-druid/pull/6068#discussion_r206212482
##
File path:
indexing-service/src/main/java/io/druid/indexing/common/task/Tasks.java
##
@@ -74,7 +73,8 @@
toBeAccumulated.add(interval);
} else {
compactIntervals.add(JodaUtils.umbrellaInterval(toBeAccumulated));
- toBeAccumulated = Lists.newArrayList(interval);
+ toBeAccumulated.clear();
Review comment:
Joda Interval objects are immutable so I think this point is moot. It
doesn't matter if `JodaUtils.umbrellaInterval` returns a new Interval instance
or not.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
