flink icon indicating copy to clipboard operation
flink copied to clipboard

[FLINK-34239][core] Add copy() method for SerializerConfig

Open kumar-mallikarjuna opened this issue 1 year ago • 4 comments

What is the purpose of the change

org.apache.flink.table.catalog.DataTypeFactoryImpl#createSerializerConfig is currently manually making a copy of a SerializerConfig object. This is cumbersome and error-prone. This change moves this copying logic inside a copy method in SerializerConfig(Impl).

Brief change log

  • A copy method is introduced in SerializerConfig and SerializerConfigImpl.
  • DataTypeFactoryImpl#createSerializerExecutionConfig reuses this method.

Verifying this change

This change added tests and can be verified as follows:

  • Added a unit test for SerializerConfigImply.copy()

Does this pull request potentially affect one of the following parts:

  • Dependencies (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (don't know - changes the underlying implementation but no functional change to the serializers)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

kumar-mallikarjuna avatar Mar 20 '24 13:03 kumar-mallikarjuna

CI report:

  • dbcf20e77ee17d5a7954ed1d4d2bd65caf8752b0 UNKNOWN
  • c6cc76501f2975282d4915de8a9b2d5f857e0e63 Azure: SUCCESS
Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

flinkbot avatar Mar 20 '24 13:03 flinkbot

@kumar-mallikarjuna Thanks for the contribution. LGTM except for two minor comments, PTAL.

X-czh avatar Mar 28 '24 12:03 X-czh

@X-czh , thanks for reviewing. I've updated the PR.

kumar-mallikarjuna avatar Mar 30 '24 13:03 kumar-mallikarjuna

Pinging @FangYongs and @reswqa. Could you help take a look when you have time?

X-czh avatar Apr 09 '24 11:04 X-czh

Hi @reswqa , I've rebased the branch. Thanks!

kumar-mallikarjuna avatar May 27 '24 07:05 kumar-mallikarjuna

@kumar-mallikarjuna Thanks for the rebase, could you squash these to some meaningful commits? I will merge this PR then.

reswqa avatar May 30 '24 04:05 reswqa

Done, @reswqa .

kumar-mallikarjuna avatar May 30 '24 10:05 kumar-mallikarjuna

Sorry for the delay, I shall merge this after CI green(A few days ago, there was a problem with CI pipelline, which has been in the UNKNOWN state. I rebase the master to re-trigger).

reswqa avatar Jun 07 '24 02:06 reswqa

Thanks @reswqa !

kumar-mallikarjuna avatar Jun 07 '24 06:06 kumar-mallikarjuna