-
Notifications
You must be signed in to change notification settings - Fork 67
[FLINK-37688] Implement Amazon CloudWatch Metric Sink Connector #202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
2d0309c
to
157fccd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @darenwkt! Left some comments
flink-connector-aws-e2e-tests/flink-connector-cloudwatch-e2e-tests/pom.xml
Outdated
Show resolved
Hide resolved
...-cloudwatch/src/main/java/org/apache/flink/connector/cloudwatch/sink/MetricWriteRequest.java
Outdated
Show resolved
Hide resolved
...-cloudwatch/src/main/java/org/apache/flink/connector/cloudwatch/sink/MetricWriteRequest.java
Outdated
Show resolved
Hide resolved
...-cloudwatch/src/main/java/org/apache/flink/connector/cloudwatch/sink/MetricWriteRequest.java
Show resolved
Hide resolved
...va/org/apache/flink/connector/cloudwatch/sink/DefaultMetricWriteRequestElementConverter.java
Outdated
Show resolved
Hide resolved
private final SdkAsyncHttpClient httpClient; | ||
private final CloudWatchAsyncClient cloudWatchAsyncClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do these need to be class level variables? could we just expose the buildClient method instead. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, I think the benefit of using getClient
is that the same cloudwatchClient instance can be reused many times in the same provider, so I am inclined to keep this for now, let me know what you think
|
||
/** Provides a {@link SdkClient}. */ | ||
@Internal | ||
public interface SdkClientProvider<T extends SdkClient> extends SdkAutoCloseable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need this interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need it strictly, I just saw that we use this for DDB and SQS sink as well and thought of using it here to be consistent
Purpose of the change
Implement Amazon CloudWatch Metric Sink Connector based on FLIP-524 (https://cwiki.apache.org/confluence/display/FLINK/FLIP-524:+CloudWatch+Metric+Sink+Connector) which includes following:
Verifying this change
This change added tests and can be verified as follows:
Unit Test
IT Test - Added integration tests for end-to-end deployment
DataStream API Manual Test - Manually verified by running the CloudWatch connector on a local Flink cluster.
Verified job is running and checkpointing successfully

Verified CloudWatch received the metric successfully

TableAPI Manual Test - Manually verified by running the CloudWatch connector on a local Flink cluster.
Verified job entered FINISHED state successfully

Verified Cloudwatch received the metric successfully

Significant changes
(Please check any boxes [x] if the answer is "yes". You can first publish the PR and check them afterwards, for convenience.)
@Public(Evolving)
)