Fix semantics of skip output#2468
Conversation
| protected void validateFunctionConfigs(FunctionConfig functionConfig) { | ||
|
|
||
| if (isBlank(functionConfig.getOutput()) && !functionConfig.isSkipOutput()) { | ||
| throw new ParameterException( |
There was a problem hiding this comment.
There was a problem hiding this comment.
skipOutput flag is currently very misleading. From the description it seems that you can run a function and pass this flag to suppress any output. Thus if you provide an output topic and specify skipOutput, this flag will nullify the output topic. Maybe I can print a message while nulliyfing the output topic.
Otherwise I really don't understand the need for skipOutput. One other thing that we can do that if output topic is not set, don;'t write it. That might be a cleaner/clearer way than this whole outputTopic and skipOutput combination. What do you think @sijie @rdhabalia @jerrypeng
There was a problem hiding this comment.
didn't we agree on #2261 to fail-fast if no output specified and if user want to skip output, they should skip-output?
|
We might have, however the moot point of this pr is that the skipOutput flag has become confusing. What do we really want to achieve here? If users dont want to output anything, they can always set their output topic to be empty or not set it at all. Why is there a need for extra flag? |
:-) that's true.. earlier motivation and change of #2261 was to not publish if output topic is empty.. but then I think you proposed to introduce flag explicitly when user doesn't want to publish in output topic.? I think then we added flag based on feedback. I am fine with removing flag but then it seems we had changed earlier PR with the feedback so, not sure if we want to add flag again in future.
|
|
IIRC, in 2.1, if you dont specify an output topic, a topic was created for you as the function output. I believe we wanted to change that by not doing that. So now if you dont specify an output topic, nothing gets created. Now I'm not sure how that got mixed up with the flag. Anyways I will remove this flag for now since its superflous |
|
retest this please |
|
run java8 tests |
1 similar comment
|
run java8 tests |
|
run integration tests |
|
run java8 tests |
|
run integration tests |
1 similar comment
|
run integration tests |
|
run java8 tests |
|
run java8 tests |
1 similar comment
|
run java8 tests |
|
run integration tests |
4 participants
Motivation
skipOutput is meant to supress any output if set to true. The current check are inaccurate in two respects
Modifications
Describe the modifications you've done.
Result
After your change, what will change.