kfaraz commented on code in PR #15463:
URL: https://github.com/apache/druid/pull/15463#discussion_r1410372717
##########
processing/src/main/java/org/apache/druid/java/util/common/granularity/Granularity.java:
##########
@@ -228,19 +228,13 @@ public boolean isFinerThan(Granularity g)
/**
* Return an iterable of granular buckets that overlap a particular interval.
*
- * In cases where the number of granular buckets is very large, the Iterable
returned by this method will take
- * an excessive amount of time to compute, and materializing it into a
collection will take an excessive amount
- * of memory. For example, this happens in the extreme case of an input
interval of
- * {@link org.apache.druid.java.util.common.Intervals#ETERNITY} and any
granularity other than
- * {@link Granularities#ALL}, as well as cases like an input interval of ten
years with {@link Granularities#SECOND}.
- *
- * To avoid issues stemming from large numbers of buckets, this method
should be avoided, and code that uses
- * this method should be rewritten to use some other approach. For example:
rather than computing all possible
- * buckets in a wide time range, only process buckets related to actual data
points that appear.
+ * A limit is passed as an argument to limit the number of intervals
returned. An {@link IllegalStateException} is
+ * thrown during iteration if the number of intervals returned exceeds the
limit. There is no default limit, and it
+ * is up to the caller to decide what that limit should be based on the use
case.
Review Comment:
Nit: Better to write this in the traditional javadoc style. For example,
```suggestion
* @param limit Maximum number of intervals to return. This parameter has
no default value.
* @throws IllegalStateException if the number of intervals generated by
this method returned exceeds the limit.
```
##########
processing/src/main/java/org/apache/druid/java/util/common/granularity/Granularity.java:
##########
@@ -228,19 +228,13 @@ public boolean isFinerThan(Granularity g)
/**
* Return an iterable of granular buckets that overlap a particular interval.
*
- * In cases where the number of granular buckets is very large, the Iterable
returned by this method will take
- * an excessive amount of time to compute, and materializing it into a
collection will take an excessive amount
- * of memory. For example, this happens in the extreme case of an input
interval of
- * {@link org.apache.druid.java.util.common.Intervals#ETERNITY} and any
granularity other than
- * {@link Granularities#ALL}, as well as cases like an input interval of ten
years with {@link Granularities#SECOND}.
- *
- * To avoid issues stemming from large numbers of buckets, this method
should be avoided, and code that uses
- * this method should be rewritten to use some other approach. For example:
rather than computing all possible
- * buckets in a wide time range, only process buckets related to actual data
points that appear.
+ * A limit is passed as an argument to limit the number of intervals
returned. An {@link IllegalStateException} is
+ * thrown during iteration if the number of intervals returned exceeds the
limit. There is no default limit, and it
+ * is up to the caller to decide what that limit should be based on the use
case.
Review Comment:
We should also mention here why having such a limit is important so that
someone doesn't accidentally remove it in the future.
--
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]
