Skip to content
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-36406] Close MetadataApplier when the job stops #3623

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

morozov
Copy link
Contributor

@morozov morozov commented Sep 29, 2024

This pull request implements closing MetadataApplier when the job stops. This allows MetadataApplier implementations to release the resources that they may use to apply schema changes (e.g. a JDBC connection).

Choice of the interface

  1. SchemaRegistryRequestHandler (which will close MetadataApplier) implements Closeable, so it would make sense to declare MetadataApplier as Closeable as well.
  2. PaimonMetadataApplier internally instantiates a Paimon Catalog, which it ideally should close, and which is Autocloseable.

To me, it makes more sense to declare MetadataApplier as AutoCloseable. The reason is that Closeable is an IO-specific interface, which is declared in java.io and throws IOException in close(). AutoCloseable is more generic and seems to be more suitable for MetadataApplier, whose implementations doesn't necessarily perform IO.

Testing

I didn't find existing tests focusing on SchemaRegistryRequestHandler or SchemaRegistry, so I dind't implement a unit test. Testing of the changes in SchemaRegistryRequestHandler can be done by implementing some logging in ValuesMetadataApplier#close and running any test that uses it (e.g. FlinkPipelineComposerITCase)

Additional scope

Just as a demonstration of where this new functionality can be applied, I implemented close() in PaimonMetadataApplier. I didn't test it and am not sure that this is the right thing to do. I can revert this part, if necessary.

Further considerations

The problem of closing resources seems also relevant for the MetadataAccessor interface. Specifically, MySqlMetadataAccessor disconnects from the database after each call but some other implementations may require maintaining a persistent connection.

@morozov morozov force-pushed the FLINK-36406-close-metadata-applier branch from 5761577 to 5ec21a4 Compare September 29, 2024 21:57
@morozov morozov changed the title [FLINK-3640] Close MetadataApplier when the job stops [FLINK-36406] Close MetadataApplier when the job stops Sep 29, 2024
Copy link
Contributor

@yuxiqian yuxiqian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for @morozov's great work! Just left some minor comments.

Comment on lines +54 to +56
@Override
default void close() throws Exception {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a JavaDocs for this method would be nice.

Comment on lines 351 to 360

try {
metadataApplier.close();
} catch (Exception e) {
throw new IOException("Failed to close metadata applier.", e);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested locally with a dummy MetadataApplier that always throws exception but gets no output. Seems OperatorCoordinators will be closed by calling OperatorCoordinatorHandler#disposeAllOperatorCoordinators, where exceptions thrown here are ignored without being logged:

// DefaultOperatorCoordinatorHandler.java
@Override
public void disposeAllOperatorCoordinators() {
    coordinatorMap.values().forEach(IOUtils::closeQuietly);
}

// IOUtils.java
public static void closeQuietly(AutoCloseable closeable) {
    try {
        if (closeable != null) {
            closeable.close();
        }
    } catch (Throwable ignored) {
    }
}

Maybe we can add some logging message if metadata appliers are not closed as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 👍 Thanks for the detailed explanation.

@morozov morozov force-pushed the FLINK-36406-close-metadata-applier branch from 5ec21a4 to f800a47 Compare September 30, 2024 04:32
Copy link
Contributor

@yuxiqian yuxiqian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! cc @ruanhang1993 @lvyanquan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants