pnowojski commented on code in PR #23550:
URL: https://github.com/apache/flink/pull/23550#discussion_r1372805309
##########
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamConfig.java:
##########
@@ -368,7 +373,8 @@ public void setStreamOperator(StreamOperator<?> operator) {
public void setStreamOperatorFactory(StreamOperatorFactory<?> factory) {
if (factory != null) {
- toBeSerializedConfigObjects.put(SERIALIZEDUDF, factory);
+ toBeSerializedConfigObjects.put(SERIALIZED_UDF, factory);
+ config.setString(SERIALIZED_UDF_CLASS_NAME,
factory.getClass().getName());
Review Comment:
> If it will be added in the future, this check will throw exception, and
correponding developer will find it, right?
>
> If so, I think it's fine, and it's better to add some comments to explain
why.
Yes :)
> Also, how about just adding the solution 1 to master branch?
That's perfectly fine. In 1.17 1.18 branches it's extremely unlikely that
anyone will ever introduce new sink factory.
> I have a question, is there the same classloader issue for solution1?
Have you noticed some issues? I wouldn't expect anything. As I said, a lot
of things would brake down, if user class loader would have loaded a different
`SinkWriterOperatorFactory` vs Flink's root class loader. It's not only that
`instanceof` would fail, but you wouldn't be able to assign the user loaded
class to many runtime variables/fields.
First and foremost, user code shouldn't include SPI classes. Those
dependencies should be declared as `provided`. Secondly, if I remember
correctly, as a precaution we are using `ParentFirstClassLoader` for any
Flink's classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
