PatrickRen commented on code in PR #21065:
URL: https://github.com/apache/flink/pull/21065#discussion_r999021853
##########
flink-connectors/flink-connector-aws-kinesis-firehose/src/main/java/org/apache/flink/connector/firehose/sink/KinesisFirehoseSinkWriter.java:
##########
@@ -96,11 +96,7 @@ private static FirehoseAsyncClient createFirehoseClient(
RESOURCE_NOT_FOUND_EXCEPTION_CLASSIFIER,
getSdkClientMisconfiguredExceptionClassifier());
- // deprecated, use numRecordsSendErrorsCounter instead.
- @Deprecated private final Counter numRecordsOutErrorsCounter;
-
- /* A counter for the total number of records that have encountered an
error during put */
- private final Counter numRecordsSendErrorsCounter;
Review Comment:
I'm not sure if I understand your argument. Please correct me if I'm wrong.
Firstly, we need to keep only one counter variable (no matter `sendCounter`
or `outCounter`) in sink writer implementations, as these two variables are
pointing to the same underlying counter actually, and
`InternalSinkWriterMetricGroup` registers the underlying counter twice in the
registry with two different metric names `out` and `send`. Keeping two
variables and increasing both of them will finally count twice towards the same
underlying counter.
Then about which variable to remove. I think what we are arguing with is
just the name of the variable in sink writer's implementation. From the user's
perspective, the result is the same no matter whether the variable name is
`sendCounter` or `outCounter` or `fooCounter`: the sink will register two
metrics `numXXXSend` and `numXXXOut` having the same value. I think it's better
to respect the original definition in FLIP-33 to use `out` to name variables
for consistency.
--
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]
