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

chore(core): add config hot-reload subsystem and implementation for pgwire credential changes #4168

Draft
wants to merge 131 commits into
base: master
Choose a base branch
from

Conversation

sklarsa
Copy link
Contributor

@sklarsa sklarsa commented Jan 27, 2024

This swaps out PropServerConfiguration usage with ReloadingPropServerConfiguration. It should be a no-op, in preparation for hot-reloading config

Instead of inheriting from PropServerConfiguration, I decided to wrap it in a new class to avoid moving code around too much, since the validation logic is in the PropServerConfiguration constructor

@sklarsa sklarsa changed the title chore(core): Replaces PropServerConfiguration with ReloadingPropServerConfiguration chore(core): replaces PropServerConfiguration with ReloadingPropServerConfiguration Jan 29, 2024
@sklarsa sklarsa changed the title chore(core): replaces PropServerConfiguration with ReloadingPropServerConfiguration chore(core): replace PropServerConfiguration with ReloadingPropServerConfiguration Jan 29, 2024
@ideoma
Copy link
Collaborator

ideoma commented Feb 2, 2024

[PR Coverage check]

😍 pass : 38 / 57 (66.67%)

file detail

path covered line new line coverage
🔵 io/questdb/ReloadingPropServerConfiguration.java 34 53 64.15%
🔵 io/questdb/PropBootstrapConfiguration.java 1 1 100.00%
🔵 io/questdb/cairo/DefaultCairoConfiguration.java 3 3 100.00%

@sklarsa
Copy link
Contributor Author

sklarsa commented May 29, 2024

@bluestreak01 here's an update on this.

I just merged master into this branch, fixed merge conflicts, and all tests pass (with the exception of the flaky TestPgWireCredentialsReloadWithNewProp, related to this PR).

It looks like I was in the process of moving my inotify code into java to take advantage of the epoll helpers that we have. But then I moved on to other cloud-related things.

The Bootstrap and ServerMain plumbing code is working as expected for the DynamicServerConfiguration. I also have the DynamicUsernamePasswordMatcher which takes advantage of the dynamic server configuration for hot-reloading PGWire credentials.

As for the cross-platform filewatcher stuff, here is where we are.

OS Method Status
Linux Inotify In progress
Osx Kqueue Needs review (and recompilation)
Freebsd Kqueue Needs review (and recompilation)
Windows ? Not started

I'm happy to sync with you on this when you have time. I'd love to finally finish this thing.

@CLAassistant
Copy link

CLAassistant commented May 31, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 4 committers have signed the CLA.

✅ bluestreak01
✅ sklarsa
✅ jerrinot
❌ GitHub Actions - Rebuild Native Libraries


GitHub Actions - Rebuild Native Libraries seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

sklarsa and others added 10 commits May 31, 2024 16:43
on close() the closing thread writes into a pipe. this is meant
to unblock the blocked polling thread. in order for this to work
we have to register to receive read-ready events from the pipe.

this way the closing thread set the `closed` flag and writes into
the pipe. this notifies the poller, which checks the closed()
flag, realizes it's closed, exit waitForChange() and finally
unblock the latch the closing thread is waiting for.
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

5 participants