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

[Fix][Kafka-Sink] fix kafka sink factory option rule #6657

Merged
merged 3 commits into from
Apr 12, 2024

Conversation

liunaijie
Copy link
Contributor

Purpose of this pull request

the current kafka sink factory option rule has some wrong usage.

from the current code, topic only required when format is in (json, cancal_json, text, ....), but it should be required whether the format is.
and we don't need limit the format value, because it's an enum, if pass an wrong value, it will get error when parse value. and all of the format is supported

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

@@ -39,17 +36,9 @@ public String factoryIdentifier() {
@Override
public OptionRule optionRule() {
return OptionRule.builder()
.required(Config.FORMAT, Config.BOOTSTRAP_SERVERS)
.conditional(
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this? conditional option can help SeaTunnel Web auto create connector form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because i find this rule is only check topic parameter exist when format is in (json, cancal_json, text, ....), but it required whether the format is. so i remove it.
if the web need use this, i will add this conditional option back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why remove this? conditional option can help SeaTunnel Web auto create connector form.

@Hisoka-X i remove it before, but eric said it will be used in web. so i put it back and move the topic option check in conditional, just put it to required

EricJoy2048
EricJoy2048 previously approved these changes Apr 9, 2024
Config.KAFKA_CONFIG,
Config.ASSIGN_PARTITIONS,
Config.TRANSACTION_PREFIX,
Config.SEMANTICS,
Config.PARTITION,
Config.PARTITION_KEY_FIELDS)
.conditional(
Copy link
Contributor

Choose a reason for hiding this comment

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

why requiredOptions is null?

Copy link
Contributor Author

@liunaijie liunaijie Apr 10, 2024

Choose a reason for hiding this comment

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

move topic to required check. if in the conditional, it only required when format type is in the list. if other format is add and using this format, it doesn't required topic parameter, then in the next steps will get exception

Copy link
Member

Choose a reason for hiding this comment

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

So as you said, I think we should remove this conditional.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I made a mistake. I have reviewed the code for seatunnel-web again and can delete the option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.
I have some question about the optional rule. we know if in required, then we must pass the parameter. if in optional, we can pass or not pass this parameter.
But we still can pass some unknown parameter to the config, like this

sink {
  kafka {
     topic = ""
     bootstrap.servers = ""
     k1 = v1
     k2 = v2
  }
}

i pass 2 parameter k1 and k2. it is not in option list and won't be use in feature.
So what's the option rule value? it can't control anything

Copy link
Member

Choose a reason for hiding this comment

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

It can verify the legality of the value corresponding to a meaningful key, thereby ensuring the normal operation of the job, and that unexpected values can be fed back to the user as early as possible. But for useless keys, not telling users to delete them is based on two principles: 1. These keys will not affect the normal operation of the job. 2. If we delete a key, the key may still exist in the user's own config. We hope that the user can run job normally without changing the config.

Copy link
Member

@Hisoka-X Hisoka-X left a comment

Choose a reason for hiding this comment

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

LGTM

@EricJoy2048 EricJoy2048 merged commit 37578e1 into apache:dev Apr 12, 2024
7 checks passed
lostinwind added a commit to lostinwind/dataGovernance that referenced this pull request Apr 15, 2024
@liunaijie liunaijie deleted the kafka-sink-factory branch April 19, 2024 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants