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

Make ClassLoaderResourceAccessor implement Closable #2308

Merged
merged 4 commits into from Jan 26, 2022

Conversation

Delir4um
Copy link
Contributor

@Delir4um Delir4um commented Dec 31, 2021

Description

The ClassLoaderResourceAccessor opens resources but has no method for closing them. Add a close() method to clean it up as part of the AutoClosabable interface


Dev Handoff Notes (Internal Use)

Links

Testing

  • Steps to Reproduce: None
  • Guidance:
    • Impact: This adds a close method which our code does not use. 3rd party integrations would be able to take advantage of it, however.

Dev Verification

Reviewed code

┆Issue is synchronized with this Jira Bug by Unito

@kataggart kataggart added this to To Do in Conditioning++ via automation Jan 4, 2022
@kataggart kataggart added TypeEnhancement RiskLow Trivial changes in spelling, documentation changes, focused bug fixes, etc. ImpactLow IntegrationAny labels Jan 4, 2022
@nvoxland nvoxland changed the title Update ClassLoaderResourceAccessor.java Make ClassLoaderResourceAccessor implement Closable Jan 5, 2022
@nvoxland nvoxland changed the base branch from master to 1_9 January 5, 2022 21:30
@nvoxland nvoxland changed the base branch from 1_9 to master January 5, 2022 21:30
Copy link
Contributor

@nvoxland nvoxland left a comment

Choose a reason for hiding this comment

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

I added a log message when close failed, but otherwise good. Thanks!

@nvoxland
Copy link
Contributor

nvoxland commented Jan 5, 2022

Just making it auto-closable doesn't help the existing code which should be calling the method.

But, in looking through the code we have there is often too much of a disconnect between when the ClassLoaderResourceAccessor is opened/created and when it would need to be closed. So I'm good with just adding the method for now. I think that over the next few months as we do more refactoring of the code we can take better advantage of this new close method. And possibly consider adding it further up the ResourceAccessor chain

@nvoxland nvoxland moved this from To Do to Code Review in Conditioning++ Jan 5, 2022
@suryaaki2 suryaaki2 moved this from Code Review to Ready for Handoff (In JIRA) in Conditioning++ Jan 6, 2022
@kataggart
Copy link
Contributor

@nvoxland please note this was approved as ready to merge

@nvoxland nvoxland merged commit 6fcabdc into liquibase:master Jan 26, 2022
Conditioning++ automation moved this from Ready for Handoff (In JIRA) to Done Jan 26, 2022
@nvoxland nvoxland added this to the v4.8.0 milestone Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ImpactLow IntegrationAny RiskLow Trivial changes in spelling, documentation changes, focused bug fixes, etc. SyncTicket TypeEnhancement
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants