[Improvement] BaseCatalog.close() skips remaining cleanup when an earlier close fails

[justinmclean]

What would you like to be improved?

BaseCatalog.close() stops closing resources after the first exception. In BaseCatalog.java (line 289), ops, authorizationPlugin, and catalogCredentialManager are closed sequentially, but if ops.close() or authorizationPlugin.close() throws, the remaining resources are never closed. This can leave plugin or credential-manager resources leaked during catalog shutdown.

How should we improve?

Make BaseCatalog.close() attempt to close all managed resources even if one fails. A straightforward fix is to catch exceptions for each close operation, continue closing the remaining resources.

Here's a unit test to help:

  @Test
  void testCloseClosesAllResourcesWhenOpsCloseFails() throws IllegalAccessException, IOException {
    TestCatalog catalog = new TestCatalog();
    CatalogOperations ops = Mockito.mock(CatalogOperations.class);
    AuthorizationPlugin authorizationPlugin = Mockito.mock(AuthorizationPlugin.class);
    CatalogCredentialManager credentialManager = Mockito.mock(CatalogCredentialManager.class);

    Mockito.doThrow(new IOException("close ops failed")).when(ops).close();

    FieldUtils.writeField(catalog, "ops", ops, true);
    FieldUtils.writeField(catalog, "authorizationPlugin", authorizationPlugin, true);
    FieldUtils.writeField(catalog, "catalogCredentialManager", credentialManager, true);

    Assertions.assertThrows(IOException.class, catalog::close);

    Mockito.verify(authorizationPlugin).close();
    Mockito.verify(credentialManager).close();
  }

[sachinnn99]

I'd like to work on this issue.