AWS: Add SQS MetricsReporter #7444

rajarshisarkar · 2023-04-27T05:31:18Z

This PR adds support for AWS SQS MetricsReporter.

Spark SQL launch command:

sh spark-sql --conf spark.sql.extensions=org.apache.iceberg.spark.extensions.IcebergSparkSessionExtensions \
    --conf spark.sql.catalog.my_catalog=org.apache.iceberg.spark.SparkCatalog \
    --conf spark.sql.catalog.my_catalog.catalog-impl=org.apache.iceberg.aws.glue.GlueCatalog \
    --conf spark.sql.catalog.my_catalog.warehouse=s3://<warehouse-path> \
    --conf spark.sql.catalog.my_catalog.metrics-reporter-impl=org.apache.iceberg.aws.metrics.SqsMetricsReporter \
    --conf spark.sql.catalog.my_catalog.metrics-reporter.sqs.queue-url=<sqs-queue-url>

cc: @jackye1995 @amogh-jahagirdar @singhpk234

amogh-jahagirdar

Thanks for publishing @rajarshisarkar had some comments, also can we add unit tests?

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java

aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java

jackye1995 · 2023-04-27T13:47:28Z

Feels like we should use the async client for this? We should not be blocked by metrics reporting calls

rajarshisarkar · 2023-04-28T08:48:59Z

Feels like we should use the async client for this? We should not be blocked by metrics reporting calls

This would mean we introduce the AsyncHttp clients too: https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/http-configuration.html#http-clients-available. Thoughts?

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java

rajarshisarkar · 2023-05-30T08:38:17Z

Feels like we should use the async client for this? We should not be blocked by metrics reporting calls

Added NettyNioAsyncHttpClient.

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsClientFactory.java

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporterProperties.java

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java

aws/src/test/java/org/apache/iceberg/aws/metrics/SqsMetricsReporterTest.java

rajarshisarkar · 2023-06-07T12:16:37Z

Introduced SqsMetricsReporterAwsClientFactory.

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java

jackye1995 · 2023-06-08T15:46:27Z

aws/src/main/java/org/apache/iceberg/aws/HttpClientProperties.java

+   * <p>For more details, see
+   * https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClient.Builder.html
+   */
+  public static final String NETTYNIO_WRITE_TIMEOUT = "http-client.nettynio.write-timeout";


nit: can you add time units to this, like -ms?

jackye1995 · 2023-06-08T15:46:31Z

aws/src/main/java/org/apache/iceberg/aws/HttpClientProperties.java

+   * <p>For more details, see
+   * https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClient.Builder.html
+   */
+  public static final String NETTYNIO_READ_TIMEOUT = "http-client.nettynio.read-timeout";


nit: can you add time units to this, like -ms?

jackye1995 · 2023-06-08T15:48:01Z

aws/src/main/java/org/apache/iceberg/aws/SqsMetricsReporterAwsClientFactories.java

+        PropertyUtil.propertyAsString(
+            properties,
+            SqsMetricsReporterProperties.METRICS_REPORTER_SQS_CLIENT_FACTORY_IMPL,
+            DefaultSqsMetricsReporterAwsClientFactory.class.getName());


similar to the old client factory, we should be able to make DefaultSqsMetricsReporterAwsClientFactory a singleton, and directly return that if METRICS_REPORTER_SQS_CLIENT_FACTORY_IMPL is not set.

jackye1995 · 2023-06-08T15:48:33Z

aws/src/main/java/org/apache/iceberg/aws/metrics/DefaultSqsMetricsReporterAwsClientFactory.java

+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import software.amazon.awssdk.services.sqs.SqsAsyncClient;
+
+public class DefaultSqsMetricsReporterAwsClientFactory


this can be an inner package protected class in the factories class.

jackye1995 · 2023-06-08T15:50:37Z

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java

+        message = ScanReportParser.toJson((ScanReport) report);
+      }
+
+      if (null == message) {


this can be just else clause of the last clause? The parsers cannot return null.

jackye1995 · 2023-06-08T15:52:25Z

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java

+              }
+            }
+          });
+    } catch (Exception e) {


can we at least use RuntimeException? Exception is too generic

jackye1995 · 2023-06-08T15:52:58Z

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java

+            }
+          });
+    } catch (Exception e) {
+      LOG.warn("Failed to report metrics to SQS queue {}", sqsQueueUrl, e);


I think this deserves a LOG.error?

jackye1995 · 2023-06-08T15:54:14Z

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java

+      future.whenComplete(
+          (response, error) -> {
+            if (response != null) {
+              LOG.info("Metrics {} reported to: {}", response, sqsQueueUrl);


I don't think we need to log the response? Just logging error when it exists seems sufficient to me, what do you think?

jackye1995 · 2023-06-08T15:54:35Z

aws/src/test/java/org/apache/iceberg/aws/metrics/SqsMetricsReporterTest.java

+  }
+
+  @Test
+  public void report() {


nit: use testXXX for testing methods

jackye1995 · 2023-06-08T15:55:10Z

aws/src/test/java/org/apache/iceberg/aws/metrics/SqsMetricsReporterTest.java

+
+  @Test
+  public void report() {
+    sqsMetricsReporter.report(commitReport);


can we test both scan and commit reports, and more test cases for success and failure cases that you are catching in the implementation?

github-actions · 2024-08-29T00:14:15Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

github-actions · 2024-09-05T00:14:21Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

github-actions bot added AWS build labels Apr 27, 2023

amogh-jahagirdar reviewed Apr 27, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java Outdated Show resolved Hide resolved

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java Outdated Show resolved Hide resolved

jackye1995 reviewed Apr 27, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java Outdated Show resolved Hide resolved

amogh-jahagirdar reviewed Apr 27, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java Outdated Show resolved Hide resolved

nastra reviewed Apr 27, 2023

View reviewed changes

rajarshisarkar force-pushed the aws-metrics-reporter branch from 269d051 to 2ecb6b5 Compare April 28, 2023 08:42

nastra reviewed Apr 28, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java Outdated Show resolved Hide resolved

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java Outdated Show resolved Hide resolved

rajarshisarkar force-pushed the aws-metrics-reporter branch 3 times, most recently from 172fca2 to f18775e Compare May 30, 2023 08:34

jackye1995 reviewed Jun 2, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsClientFactory.java Outdated Show resolved Hide resolved

jackye1995 reviewed Jun 2, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java Outdated Show resolved Hide resolved

jackye1995 reviewed Jun 2, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporterProperties.java Outdated Show resolved Hide resolved

jackye1995 reviewed Jun 2, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java Outdated Show resolved Hide resolved

jackye1995 reviewed Jun 2, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java Outdated Show resolved Hide resolved

jackye1995 reviewed Jun 2, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java Show resolved Hide resolved

nastra reviewed Jun 3, 2023

View reviewed changes

aws/src/test/java/org/apache/iceberg/aws/metrics/SqsMetricsReporterTest.java Outdated Show resolved Hide resolved

rajarshisarkar force-pushed the aws-metrics-reporter branch 2 times, most recently from bf2cabc to 5812918 Compare June 7, 2023 12:16

rajarshisarkar force-pushed the aws-metrics-reporter branch 2 times, most recently from 73d443e to 9d4fd29 Compare June 7, 2023 13:11

jackye1995 reviewed Jun 7, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java Outdated Show resolved Hide resolved

jackye1995 reviewed Jun 7, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/metrics/SqsMetricsReporter.java Outdated Show resolved Hide resolved

AWS: Add SQS MetricsReporter

9c919ce

rajarshisarkar force-pushed the aws-metrics-reporter branch from 9d4fd29 to 9c919ce Compare June 8, 2023 08:36

jackye1995 reviewed Jun 8, 2023

View reviewed changes

github-actions bot added the stale label Aug 29, 2024

github-actions bot closed this Sep 5, 2024

AWS: Add SQS MetricsReporter #7444

AWS: Add SQS MetricsReporter #7444

Uh oh!

Conversation

rajarshisarkar commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jackye1995 commented Apr 27, 2023

Uh oh!

rajarshisarkar commented Apr 28, 2023

Uh oh!

Uh oh!

Uh oh!

rajarshisarkar commented May 30, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rajarshisarkar commented Jun 7, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackye1995 Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 29, 2024

Uh oh!

github-actions bot commented Sep 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rajarshisarkar commented Apr 27, 2023 •

edited

Loading

jackye1995 Jun 8, 2023 •

edited

Loading