flink icon indicating copy to clipboard operation
flink copied to clipboard

[FLINK-34463] Open catalog in CatalogManager should use proper context classloader

Open jrthe42 opened this issue 1 year ago • 1 comments

What is the purpose of the change

When we try to create a catalog in CatalogManager, if the catalog jar is added using ADD JAR and the catalog itself requires SPI mechanism, the operation may fail.

This PR try to fix this issue.

Brief change log

Wrap Catalog.open with a temporary context classloader.

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:

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
  • The serializers: (yes / no / don't know)
  • The runtime per-record code paths (performance sensitive): (yes / no / don't know)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
  • The S3 file system connector: (yes / no / don't know)

Documentation

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

jrthe42 avatar Feb 19 '24 09:02 jrthe42

CI report:

  • 1c79197e7e7360cb2fac950a144571c5e2871ab9 Azure: SUCCESS
Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

flinkbot avatar Feb 19 '24 09:02 flinkbot

Thank you for updating the code. Through the unit tests you wrote, I understand the specific issues that need to be addressed. I have a question: if we pass the ClassLoader into Catalog when creating the Catalog, can using this passed ClassLoader in your custom Catalog solve this problem? cc @xuyangzhong

hackergin avatar Feb 28 '24 12:02 hackergin

Thanks for the ping, @hackergin. IIUC, in this case, CatalogStore and CatalogManager are not using the same classloader, which has led to the occurrence of this bug. In actuality, ResourceManager, CatalogManager, and CatalogStore all use the same classloader, right? When a jar is added to the ResourceManager's classloader using ADD JAR, in fact, some classes within this jar should also be accessible within CatalogManager. If there is anything incorrect, please correct me :)

xuyangzhong avatar Mar 01 '24 08:03 xuyangzhong