You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/07/21 16:50:28 UTC
[GitHub] [iceberg] nastra opened a new pull request, #5328: API: Remove Optional from Counter#count()
nastra opened a new pull request, #5328:
URL: https://github.com/apache/iceberg/pull/5328
The rationale behind this change is that usually a Counter should always
be able to report the actual count it is counting. The reason
`count()` was made `Optional` initially is because in the case of Hadoop
we are reporting the count to Hadoop's file system statistics and don't
necessarily want to report the count in that case.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5328: API: Remove Optional from Counter#count()
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5328:
URL: https://github.com/apache/iceberg/pull/5328#discussion_r927069530
##########
api/src/main/java/org/apache/iceberg/metrics/IntCounter.java:
##########
@@ -45,8 +44,8 @@ public void increment(Integer amount) {
}
@Override
- public Optional<Integer> count() {
- return Optional.of(counter.get());
+ public Integer count() {
Review Comment:
Yes, it should!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #5328: API: Remove Optional from Counter#count()
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #5328:
URL: https://github.com/apache/iceberg/pull/5328#discussion_r926986400
##########
api/src/main/java/org/apache/iceberg/metrics/IntCounter.java:
##########
@@ -45,8 +44,8 @@ public void increment(Integer amount) {
}
@Override
- public Optional<Integer> count() {
- return Optional.of(counter.get());
+ public Integer count() {
Review Comment:
Why did the `revapi` check pass with this? Shouldn't it catch this type of change?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #5328: API: Deprecate Counter#count() / Add Counter#value()
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #5328:
URL: https://github.com/apache/iceberg/pull/5328
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5328: API: Deprecate Counter#count() / Add Counter#value()
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5328:
URL: https://github.com/apache/iceberg/pull/5328#discussion_r927856938
##########
api/src/main/java/org/apache/iceberg/metrics/MetricsContext.java:
##########
@@ -66,11 +66,20 @@ default void initialize(Map<String, String> properties) {
* Reporting count is optional if the counter is reporting externally.
*
* @return current count if available
+ * @deprecated Use {@link Counter#value()}
*/
+ @Deprecated
default Optional<T> count() {
return Optional.empty();
}
+ /**
+ * Reports the current count.
+ *
+ * @return The current count
+ */
+ T value();
Review Comment:
I'd say let's add the default.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #5328: API: Deprecate Counter#count() / Add Counter#value()
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #5328:
URL: https://github.com/apache/iceberg/pull/5328#discussion_r927797469
##########
api/src/main/java/org/apache/iceberg/metrics/MetricsContext.java:
##########
@@ -66,11 +66,20 @@ default void initialize(Map<String, String> properties) {
* Reporting count is optional if the counter is reporting externally.
*
* @return current count if available
+ * @deprecated Use {@link Counter#value()}
*/
+ @Deprecated
default Optional<T> count() {
return Optional.empty();
}
+ /**
+ * Reports the current count.
+ *
+ * @return The current count
+ */
+ T value();
Review Comment:
we could make this a breaking API change and add the justification to RevAPI (as it is here) or (as @danielcweeks
suggested) change this to
```
default T value() {
throw new UnsupportedOperationException("Count is not supported.");
}
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #5328: API: Deprecate Counter#count() / Add Counter#value()
Posted by GitBox <gi...@apache.org>.
nastra commented on PR #5328:
URL: https://github.com/apache/iceberg/pull/5328#issuecomment-1192811331
@rdblue pushed the version with the default implementation for `value()`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #5328: API: Remove Optional from Counter#count()
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #5328:
URL: https://github.com/apache/iceberg/pull/5328#discussion_r927380524
##########
api/src/main/java/org/apache/iceberg/metrics/IntCounter.java:
##########
@@ -45,8 +44,8 @@ public void increment(Integer amount) {
}
@Override
- public Optional<Integer> count() {
- return Optional.of(counter.get());
+ public Integer count() {
Review Comment:
I looked a bit why Revapi didn't fail here. Turns out https://github.com/apache/iceberg/blob/master/.palantir/revapi.yml#L2 still compares the API to the 0.13.0 version, which didn't have the `Counter` API.
@rdblue can you please create `release-base-0.14.0`?
I created https://github.com/apache/iceberg/pull/5336 that tells RevAPI to check API compatibility against `release-base-0.14.0`. Additionally, `release-base-0.14.0` is required so that nightly snapshot publishing uses `0.15.0-SNAPSHOT` (currently it still does `0.14.0-SNAPSHOT`, because of https://github.com/apache/iceberg/blob/8300b219ec8962bd22af69adf67a4ab72dec1f48/build.gradle#L682).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5328: API: Remove Optional from Counter#count()
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5328:
URL: https://github.com/apache/iceberg/pull/5328#discussion_r926979715
##########
api/src/main/java/org/apache/iceberg/metrics/IntCounter.java:
##########
@@ -45,8 +44,8 @@ public void increment(Integer amount) {
}
@Override
- public Optional<Integer> count() {
- return Optional.of(counter.get());
+ public Integer count() {
Review Comment:
Since this is in API, we can't remove it without deprecation. For now, can you add it back and just always return a non-empty optional? Then we can add a new method that always returns the value, like `value()`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org