Remove the retry mechanism in MongoSinkTask #88

stIncMale · 2021-11-26T23:57:48Z

This is a subtask of KAFKA-257 created to produce smaller PRs. CHANGELOG.md will be updated once KAFKA-257 is complete, I left a reminder in this comment.

TODO for @stIncMale: When this is merged, delete the branch KAFKA-267 and change the "into" branch for #91 to master.

KAFKA-267

Running `./gradlew check` results in some files being reformatted. I don't know why these files have not been reformatted, but I think this must be done to avoid interference with future changes. KAFKA-267

KAFKA-267

stIncMale · 2021-11-27T00:03:13Z

src/main/java/com/mongodb/kafka/connect/sink/MongoSinkConfig.java

+   *     href="https://docs.mongodb.com/kafka-connector/master/sink-connector/configuration-properties/topic-override/">here</a>.
+   * @see #OBSOLETE_CONFIGS
+   */
+  static void logObsoleteProperties(final Collection<String> propertyNames) {


I am not sure if any configuration properties have been made obsolete up until now, and if so, what is the process. I think warning a user about still having these properties is useful, but if we have a different process of obsoleting configuration properties, please share.

stIncMale · 2021-11-27T00:04:26Z

src/main/java/com/mongodb/kafka/connect/util/VisibleForTesting.java

+@Documented
+@Retention(RetentionPolicy.SOURCE)
+@Target({ElementType.TYPE, ElementType.CONSTRUCTOR, ElementType.METHOD, ElementType.FIELD})
+public @interface VisibleForTesting {


No such annotation was available at compile-time, so I copied one from the MongoDB Java driver.

rozza

I think we need to provide a slightly more helpful warning message to users.

Also can you confirm the validateAll is called when running / adding the connector?

rozza · 2021-12-06T11:05:05Z

src/main/java/com/mongodb/kafka/connect/sink/MongoSinkConfig.java

+        .forEach(
+            obsoletePropertyName ->
+                LOGGER.warn(
+                    "The configuration property {} is obsolete. Remove it as it has no effect.",


Suggestion:

Retries are obsolete but we should tell the user to set retryWrites=true on their connection string. Otherwise this might cause a support burden as users ask questions about the feature.

we should tell the user to set retryWrites=true

I addressed this in the scope document:

The connector requires the Java driver 4.3.x, which has retries enabled by default, meaning that no changes are needed to utilize the mechanism.

I updated the message to reflect that a user does not need to do anything beyond removing the obsolete configuration properties.

I still think the message will lead to customer questions and therefore add support burden. It tells users what to do and not the why its no longer required.

Also although retries are enabled by default - if a user previously set retryWrites=false thinking the Kafka connector retry mechanism is preferred then they do have to do some action to enable retryable writes.

While the logic for obsolete messages is clean and DRY, the warn message is only needed if max.num.retries > 1 and max.num.retries defaults to 1.

tells users what to do and not the why its no longer required.
if a user previously set retryWrites=false thinking the Kafka connector retry mechanism is preferred then they do have to do some action to enable retryable writes.

Updated the warning message to address the above.

the warn message is only needed if max.num.retries > 1

I think that warning a user that he uses an obsolete configuration property is useful regardless of the property value. Additionally, the proposed logic

is a complication just for the sake of not showing a warning message in some situations, despite the user specifying values for obsolete configuration properties;

requires us to read and parse configuration values directly from Map<String, String> in validateAll as opposed to reading the values via the AbstractConfig API.

Accordingly, I find the proposed logic harmful rather than helpful.

stIncMale · 2021-12-06T22:26:56Z

can you confirm the validateAll is called when running / adding the connector?

The method ConfigDef.validateAll of the anonymous class defined in the method MongoSinkConfig.createConfigDef does not seem to be called by the MongoDB Kafka Connector code, but it is called by the Kafka Connect framework when it adds a sink connector. Here is how I checked:

Specified an obsolete property in the sink config in docker/run.sh.
Modified MongoSinkConfig.logObsoleteProperties such that it throws a runtime exception instead of logging, because at this point I do not know where to look for Kafka Connect logs when it is started via docker/run.sh.
Run docker/run.sh and observed that adding a sink connector returned {"error_code":500,"message":"message from the exception"}.

…solete configuration properties KAFKA-267

….sh` KAFKA-267

rozza

The warning message should be relevant and explicit as to the what action is needed and why. This will reduce any support burden.

The MAX_NUM_RETRIES_DOC and RETRIES_DEFER_TIMEOUT_DOC should also be updated for the config definition.

stIncMale · 2021-12-10T23:47:16Z

The MAX_NUM_RETRIES_DOC and RETRIES_DEFER_TIMEOUT_DOC should also be updated for the config definition.

Just trying to confirm that I understand the proposal correctly. Ross, do you want me to reinstate these two configuration properties, i.e., continue defining them via ConfigDef, but with different documentation, that states that the properties are obsolete and are not used? If this is what you are proposing, then could you tell how such a change is useful?

I see it like this:

Users who do not use the obsolete properties will still see them (wherever they are shown as a result of being defined in a ConfigDef) with an unhelpful documentation saying that they are not to be used.
Users who use the obsolete properties do not repeatedly go and read about them in the documentation produced by ConfigDef, because they already use the properties. I.e., it is very unlikely that they will read about the properties being obsolete there. Such users may and are more likely to see information about the obsolete properties in both what's new and as warning messages in logs.

Despite me not seeing this as a useful change, if you indeed proposed to reinstate the properties (I am not sure if that's the case), I will do that.

KAFKA-267

rozza

Apologies @stIncMale I appear to have viewed the source at the first commit and missed that the configs were removed from the ConfigDef.

My only suggestion would be to map the the property name to the error message used in logObsoleteProperties but that can be done when / if we obsolete any other properties.

LGTM!

stIncMale · 2021-12-14T00:44:56Z

map the the property name to the error message used in logObsoleteProperties

Yes, since the warning message is not generic anymore, we will need different messages for different properties. When I saw your idea, I thought, maybe we can even express OBSOLETE_CONFIGS as a map of property names to SLF4J message formats. But then it seemed like an overkill, and a switch statement in logObsoleteProperties looks like a more straightforward approach for the future.

stIncMale · 2021-12-14T00:58:16Z

Omg, I got used to only one option when merging a PR being available for the Java driver, and blindly pressed the button here. As a result, it merged all the commits from the PR into master :/ I apologize for producing the garbage there.

Then I went and unchecked the "Allow merge commits" and "Allow rebase merging" checkboxes in the settings tab, only "Allow squash merging " is left checked. @rozza Do you agree we will be better this way?

rozza · 2021-12-14T09:13:43Z

@stIncMale no worries - it happens :)

I generally agree squash merging is the best approach, I suppose it depends on the PR and the commits.

stIncMale added 2 commits November 22, 2021 16:22

Commit changes done by auto-formatting

cd4bc93

Running `./gradlew check` results in some files being reformatted. I don't know why these files have not been reformatted, but I think this must be done to avoid interference with future changes. KAFKA-267

Remove the retry mechanism in MongoSinkTask

c9a1844

KAFKA-267

stIncMale requested a review from rozza November 26, 2021 23:57

stIncMale self-assigned this Nov 26, 2021

stIncMale commented Nov 27, 2021

View reviewed changes

stIncMale mentioned this pull request Dec 2, 2021

Create MongoSinkTask unit tests for DQL-related behavior #91

Merged

rozza requested changes Dec 6, 2021

View reviewed changes

stIncMale added 2 commits December 6, 2021 15:39

Clarify that the user does not need to do anything beyond removing ob…

a39a4b5

…solete configuration properties KAFKA-267

Clarify where to look for RAM/CPU restrictions when using `docker/run…

34b27c9

….sh` KAFKA-267

stIncMale requested a review from rozza December 6, 2021 22:45

rozza requested changes Dec 7, 2021

View reviewed changes

Improve the warning message

a24a8cb

KAFKA-267

stIncMale force-pushed the KAFKA-267 branch from 772c5b2 to a24a8cb Compare December 10, 2021 23:48

rozza approved these changes Dec 13, 2021

View reviewed changes

stIncMale merged commit 1d6b3f7 into mongodb:master Dec 14, 2021

stIncMale deleted the KAFKA-267 branch December 14, 2021 00:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the retry mechanism in MongoSinkTask #88

Remove the retry mechanism in MongoSinkTask #88

stIncMale commented Nov 26, 2021 •

edited

stIncMale Nov 27, 2021

stIncMale Nov 27, 2021

rozza left a comment

rozza Dec 6, 2021

stIncMale Dec 6, 2021

rozza Dec 7, 2021

stIncMale Dec 10, 2021 •

edited

stIncMale commented Dec 6, 2021 •

edited

rozza left a comment

stIncMale commented Dec 10, 2021

rozza left a comment

stIncMale commented Dec 14, 2021

stIncMale commented Dec 14, 2021

rozza commented Dec 14, 2021

Remove the retry mechanism in MongoSinkTask #88

Remove the retry mechanism in MongoSinkTask #88

Conversation

stIncMale commented Nov 26, 2021 • edited

stIncMale Nov 27, 2021

Choose a reason for hiding this comment

stIncMale Nov 27, 2021

Choose a reason for hiding this comment

rozza left a comment

Choose a reason for hiding this comment

rozza Dec 6, 2021

Choose a reason for hiding this comment

stIncMale Dec 6, 2021

Choose a reason for hiding this comment

rozza Dec 7, 2021

Choose a reason for hiding this comment

stIncMale Dec 10, 2021 • edited

Choose a reason for hiding this comment

stIncMale commented Dec 6, 2021 • edited

rozza left a comment

Choose a reason for hiding this comment

stIncMale commented Dec 10, 2021

rozza left a comment

Choose a reason for hiding this comment

stIncMale commented Dec 14, 2021

stIncMale commented Dec 14, 2021

rozza commented Dec 14, 2021

stIncMale commented Nov 26, 2021 •

edited

stIncMale Dec 10, 2021 •

edited

stIncMale commented Dec 6, 2021 •

edited