-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
[FLINK-36604][datastream] StreamingJobGraphGenerator::setOperatorConfig checks input serializer length #25576
base: master
Are you sure you want to change the base?
Conversation
@sujay-jain Could you help review this patch? Appreciate your help. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a very useful change @liuml07. I have a small suggestion around improving the error message
@@ -1191,6 +1191,12 @@ public static void setOperatorConfig( | |||
? 0 // single input operator | |||
: inEdge.getTypeNumber() - 1; // in case of 2 or more inputs | |||
|
|||
Preconditions.checkState( | |||
inputIndex < inputSerializers.length, | |||
"Input type serializer of vertex '%s' was null or undefined for inputIndex %s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making the error message more descriptive by adding some more context. For example, specifying the expected number of input serializers can help with debugging. A possible improvement could be:
"Invalid inputIndex %s for vertex '%s': Expected inputIndex to be less than %s (number of input serializers)."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much, Sujay! That is very helpful feedback, and I have updated the patch to address the comments.
…ig checks input serializer length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thank you for the contribution!
Could you help review this, @reswqa ? Thank you |
Could you share the example job that make this check unhappy? The |
@reswqa Yes agreed vertex is indeed low-level concept. I think some information about the error is still useful than built-in exception
Sample:
Regarding the scenario of the failing example, the user job I met a few months ago was using a custom TypeInformation that was not implemented correctly. To be more specific, the public class MissingSerDeserInTypeInformation {
public static class MyObject {
private final Integer value;
MyObject(Integer value) {
this.value = value;
}
@Override
public String toString() {
return "MyObject(" + value + ")";
}
}
public static class MyObjectTypeInfo extends TypeInformation<MyObject> {
public boolean isBasicType() {return false;}
public boolean isTupleType() {return false;}
public int getArity() {return 0;}
public int getTotalFields() {return 0;}
public Class<MyObject> getTypeClass() {return MyObject.class;}
public boolean isKeyType() {return false;}
public TypeSerializer<MyObject> createSerializer(ExecutionConfig executionConfig) {
return null; // should always return a valid serializer
}
public String toString() {return "MyObjectTypeInfo";}
public boolean equals(Object o) {return false;}
public int hashCode() {return 0;}
public boolean canEqual(Object o) {return false;}
}
public static void main(String[] args) throws Exception {
StreamExecutionEnvironment env = StreamExecutionEnvironment.getExecutionEnvironment();
env.getConfig().disableGenericTypes();
env.fromCollection(IntStream.range(1, 20).boxed().collect(Collectors.toList()))
.map(MyObject::new)
.returns(new MyObjectTypeInfo())
.print();
env.execute();
}
} |
@flinkbot run azure |
https://issues.apache.org/jira/browse/FLINK-36604
What is the purpose of the change
We can add a precondition check and report meaningful error message for debugging.
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation