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/10/20 09:53:12 UTC
[GitHub] [iceberg] nastra opened a new pull request, #6023: Core: Don't fail scan planning if REST metric reporting fails
nastra opened a new pull request, #6023:
URL: https://github.com/apache/iceberg/pull/6023
Metrics reporting via REST could temporarily fail for multiple reasons, such as Network hiccups or timeouts. That shouldn't interfere with scan planning overall.
--
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 pull request #6023: Core: Don't fail scan planning if REST metric reporting fails
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #6023:
URL: https://github.com/apache/iceberg/pull/6023#issuecomment-1286289744
Thanks, @nastra!
--
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] hililiwei commented on a diff in pull request #6023: Core: Don't fail scan planning if REST metric reporting fails
Posted by GitBox <gi...@apache.org>.
hililiwei commented on code in PR #6023:
URL: https://github.com/apache/iceberg/pull/6023#discussion_r1000540283
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -304,12 +304,16 @@ private void reportMetrics(
MetricsReport report,
Supplier<Map<String, String>> headers) {
reporter.report(report);
- client.post(
- paths.metrics(tableIdentifier),
- ReportMetricsRequest.of(report),
- null,
- headers,
- ErrorHandlers.defaultErrorHandler());
+ try {
+ client.post(
+ paths.metrics(tableIdentifier),
+ ReportMetricsRequest.of(report),
+ null,
+ headers,
+ ErrorHandlers.defaultErrorHandler());
+ } catch (Exception e) {
+ LOG.warn("Failed to report metrics to REST endpoint for table {}", tableIdentifier, e);
Review Comment:
nit: Print out the details of the report?
--
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 #6023: Core: Don't fail scan planning if REST metric reporting fails
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6023:
URL: https://github.com/apache/iceberg/pull/6023#discussion_r1000931892
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -304,12 +304,16 @@ private void reportMetrics(
MetricsReport report,
Supplier<Map<String, String>> headers) {
reporter.report(report);
- client.post(
- paths.metrics(tableIdentifier),
- ReportMetricsRequest.of(report),
- null,
- headers,
- ErrorHandlers.defaultErrorHandler());
+ try {
+ client.post(
+ paths.metrics(tableIdentifier),
+ ReportMetricsRequest.of(report),
+ null,
+ headers,
+ ErrorHandlers.defaultErrorHandler());
+ } catch (Exception e) {
+ LOG.warn("Failed to report metrics to REST endpoint for table {}", tableIdentifier, e);
Review Comment:
it's being printed to the log right outside the try-catch block, so not sure we'd want to repeat the report details in this warning message
--
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 #6023: Core: Don't fail scan planning if REST metric reporting fails
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6023:
URL: https://github.com/apache/iceberg/pull/6023#discussion_r1000415611
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -304,12 +304,16 @@ private void reportMetrics(
MetricsReport report,
Supplier<Map<String, String>> headers) {
reporter.report(report);
Review Comment:
we don't include this line into the `try-catch` because it's just using the `LoggingMetricsReporter`. Every metrics reporter should have its own "don't-fail-scan-planning-if-reporting-fails" and below is the one we're adding for REST metric reporting
--
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 #6023: Core: Don't fail scan planning if REST metric reporting fails
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #6023:
URL: https://github.com/apache/iceberg/pull/6023
--
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