Good point! @Gordon Introducing an `InitContext#createSerializer(TypeInformation)` looks a better option to me, so we do not need to introduce an unmodifiable `ExecutionConfig` at this moment.
Hope that we can make `ExecutionConfig` a read-only interface in Flink 2.0. It is exposed in `RuntimeContext` to user functions already, while mutating the values at runtime is actually an undefined behavior. Thanks, Zhu Tzu-Li (Gordon) Tai <tzuli...@apache.org> 于2023年4月18日周二 01:02写道： > > Hi, > > Sorry for chiming in late. > > I'm not so sure that exposing ExecutionConfig / ReadExecutionConfig > directly through Sink#InitContext is the right thing to do. > > 1. A lot of the read-only getter methods on ExecutionConfig are irrelevant > for sinks. Expanding the scope of the InitContext interface with so many > irrelevant methods is probably going to make writing unit tests a pain. > > 2. There's actually a few getter methods on `InitContext` that have > duplicate/redundant info for what ExecutionConfig exposes. For example, > InitContext#getNumberOfParallelSubtasks and InitContext#getAttemptNumber > currently exist and it can be confusing if users find 2 sources of that > information (either via the `InitContext` and via the wrapped > `ExecutionConfig`). > > All in all, it feels like `Sink#InitContext` was introduced initially as a > means to selectively only expose certain information to sinks. > > It looks like right now, the only requirement is that some sinks require 1) > isObjectReuseEnabled, and 2) TypeSerializer for the input type. Would it > make sense to follow the original intent and only selectively expose these? > For 1), we can just add a new method to `InitContext` and forward the > information from `ExecutionConfig` accessible at the operator level. > For 2), would it make sense to create the serializer at the operator level > and then provide it through `InitContext`? > > Thanks, > Gordon > > On Mon, Apr 17, 2023 at 8:23 AM Zhu Zhu <reed...@gmail.com> wrote: > > > We can let the `InitContext` return `ExecutionConfig` in the interface. > > However, a `ReadableExecutionConfig` implementation should be returned > > so that exceptions will be thrown if users tries to modify the > > `ExecutionConfig`. > > > > We can rework all the setters of `ExecutionConfig` to internally invoke a > > `setConfiguration(...)` method. Then the `ReadableExecutionConfig` can > > just override that method. But pay attention to a few exceptional > > setters, i.e. those for globalJobParameters and serializers. > > > > We should also explicitly state in the documentation of > > `InitContext #getExecutionConfig()`, that the returned `ExecutionConfig` > > is unmodifiable. > > > > Thanks, > > Zhu > > > > João Boto <eskabe...@apache.org> 于2023年4月17日周一 16:51写道： > > > > > > Hi Zhu, > > > > > > Thanks for you time for reviewing this. > > > > > > Extending ´ExecutionConfig´ will allow to modify the values in the > > config (this is what we want to prevent with Option2) > > > > > > To extend the ExecutionConfig is not simpler to do Option1 (expose > > ExecutionConfig directly). > > > > > > Regards > > > > > > > > > > > > On 2023/04/03 09:42:28 Zhu Zhu wrote: > > > > Hi João, > > > > > > > > Thanks for creating this FLIP! > > > > I'm overall +1 for it to unblock the migration of sinks to SinkV2. > > > > > > > > Yet I think it's better to let the `ReadableExecutionConfig` extend > > > > `ExecutionConfig`, because otherwise we have to introduce a new method > > > > `TypeInformation#createSerializer(ReadableExecutionConfig)`. The new > > > > method may require every `TypeInformation` to implement it, including > > > > Flink built-in ones and custom ones, otherwise exceptions will happen. > > > > That goal, however, is pretty hard to achieve. > > > > > > > > Thanks, > > > > Zhu > > > > > > > > João Boto <eskabe...@apache.org> 于2023年2月28日周二 23:34写道： > > > > > > > > > > I have update the FLIP with the 2 options that we have discussed.. > > > > > > > > > > Option 1: Expose ExecutionConfig directly on InitContext > > > > > this have a minimal impact as we only have to expose the new methods > > > > > > > > > > Option 2: Expose ReadableExecutionConfig on InitContext > > > > > with this option we have more impact as we need to add a new method > > to TypeInformation and change all implementations (current exists 72 > > implementations) > > > > > > > > > > Waiting for feedback or concerns about the two options > > > > > >
