Skip to content
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

Kafka 414 #158

Closed
wants to merge 3 commits into from
Closed

Kafka 414 #158

wants to merge 3 commits into from

Conversation

rasifmahmud
Copy link
Contributor

}
});
return rawConfig;
public static Map<String, String> evaluateConfigValues(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesnt look right. Why do we want to get from originals? I thought values like in the above code is resolved configs which we want to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jagadishmdb you are right. In the above code, configValue.value(ev) replaces the unresolved value with the resolved value. But the problem is configValue has another List property named "errorMessages". And that gets populated earlier. Please Check: MongoSinkConnector -> validate -> super.validate(connectorConfigs) call. errorCheckingPasswordValueValidator method adds that error to the errorMessages property of the config when super.validate is called with raw unresolved configs.

So even though configValue.value is replaced with the resolved value, configValue.errorMessages is non empty at this point. Which eventually causes to throw the error down the line.

Now, let's talk about the below code segment. Even though I am reading from originals, originals property actually has all the resolved values (sounds confusing!). The answer is in the constructor of the AbstractConfig class

this.originals = resolveConfigVariables(configProviderProps, (Map<String, Object>) originals);

So the originals property has all the resolved values. And in my PR I am calling "super.validate" after raw config has been resolved, so the errorCheckingPasswordValueValidator does not get the opportunity to add that error message to the errorMessages property.

@jagadishmdb
Copy link
Collaborator

Pardon me, I now understand that the issue is calling super.validate before we actually try to resolve the config. I think we should move the validate for both source and sink. I do not understand why we need in the evaluate function.

@rasifmahmud
Copy link
Contributor Author

Pardon me, I now understand that the issue is calling super.validate before we actually try to resolve the config. I think we should move the validate for both source and sink. I do not understand why we need in the evaluate function.

As long as we are calling super.validate on the resolved configs, we should be good.

I think there can be other ways to transform unresolved configs to resolved configs . What I proposed is just one of them. Feel free to experiment to find a better solution. Thanks

@jagadishmdb
Copy link
Collaborator

This is added to a new pull request.

@jagadishmdb jagadishmdb closed this Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants