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

creates an abstract RefreshableServiceProviderPlace for config reloading #1035

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

Conversation

dev-mlb
Copy link
Collaborator

@dev-mlb dev-mlb commented Dec 29, 2024

This is a first cut into handling place config refreshing -- this does not handle service key changes at this time

@jpdahlke
Copy link
Collaborator

😍

*
* @throws IOException if there is an issue loading the config
*/
protected void refresh() throws IOException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It occurs to me that this might not be safe to call while the place is actively processing an IBDO, or while trying to identify the next place that should process a payload.

@cfkoehler cfkoehler added this to the v8.21.0 milestone Jan 6, 2025
@dev-mlb dev-mlb changed the title update ServiceProviderPlace to support config refresh creates an abstract RefreshableServiceProviderPlace for config reloading Jan 13, 2025
Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

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

Looks like a good approach. Minor changes suggested

return this.invalid.get();
}

public void invalidate() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Might want to add a Javadoc comment stating that invalidate must be called before calling refresh.
  2. Might want to mark this method as final and do the same to the refresh overloads

}

@Test
void testReconfigure() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider adding another test (or block within this test) that calls place.refresh() without first calling invalidate()

import javax.annotation.Nullable;

/**
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
*
* ServiceProviderPlace that supports on-demand refresh of its configuration

drivenflywheel
drivenflywheel previously approved these changes Jan 14, 2025
Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

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

LGTM. The only potential change I see might be to also mark the new isValid() and setInvalid() methods as final

drivenflywheel
drivenflywheel previously approved these changes Jan 14, 2025
@dev-mlb
Copy link
Collaborator Author

dev-mlb commented Jan 14, 2025

Sorry, the directory place change doesn't need to go out with this round of changes.

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

Successfully merging this pull request may close these issues.

4 participants