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

feat: support checking preconditions before starting core app #9026

Merged
merged 3 commits into from
Apr 20, 2022

Conversation

rodesai
Copy link
Contributor

@rodesai rodesai commented Apr 13, 2022

Description

This patch changes up the precondition checker a bit so that it
runs before the main KsqlRestApplication. This way, preconditions
can look out for critical config properties to arrive, before they
are handled by the actual application.

Detailed changes:
- Run the precondition checker as a separate Executable implementation
before running KsqlRestApplication. The precondition checker waits for
any preconditions to pass, and while this is ongoing it runs a simple
server that responds to all requests (other than the health probes)
with a 503.
- Change the precondition interface to accept a properties loader so
that precondition checkers can wait on the arrival of dynamic properties.
- Support multiple properties files as args (which are then overlaid)

Testing done

Added unit tests for the new precondition checker
Tested manually by updating a config file at runtime with a precondition plugin that looks
for such a change.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@rodesai rodesai requested a review from a team as a code owner April 13, 2022 17:33
@rodesai rodesai marked this pull request as draft April 13, 2022 17:33
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

}
return options;
}

private static void setTlsOptions(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All this code was moved over into a common utils class (ApiServerUtils)

@@ -354,51 +347,6 @@ public void shouldSendCreateStreamRequestBeforeSettingReady() {
inOrder.verify(serverState).setReady();
}

@Test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we no longer check preconditions from the rest application, we no longer need these tests.

@rodesai rodesai changed the title WIP: feat: support checking preconditions before starting core app feat: support checking preconditions before starting core app Apr 14, 2022
@rodesai rodesai marked this pull request as ready for review April 14, 2022 23:19
Copy link
Member

@stevenpyzhang stevenpyzhang left a comment

Choose a reason for hiding this comment

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

Overall makes sense to me. I think we could use some more comments for clarity on the new precondition checker executable and server.

Also, is it not possible to rework PreconditionFunctionalTest.java for this new model? We should probably have some integration test for this.

);
}

private void checkPreconditions() {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment on how this function continuously runs and determines if PreconditionServer will continue running or not? And if it passes, we'll terminate the pre condition server and move onto the main KSQL server.

Copy link
Member

@stevenpyzhang stevenpyzhang left a comment

Choose a reason for hiding this comment

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

lgtm

This patch changes up the precondition checker a bit so that it
runs before the main KsqlRestApplication. This way, preconditions
can look out for critical config properties to arrive, before they
are handled by the actuall application.

Detailed changes:
- Run the precondition checker as a separate Executable implementation
  before running KsqlRestApplication. The precondition checker waits for
  any preconditions to pass, and while this is ongoing it runs a simple
  server that responds to all requests (other than the health probes)
  with a 503.
- Change the precondition interface to accept a properties loader so
  that precondition checkers can wait on the arrival of dynamic properties.
- Support multiple properties files as args (which are then overlaid)
@rodesai rodesai merged commit 33a6a04 into confluentinc:master Apr 20, 2022
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.

None yet

3 participants