chore(core): add config hot-reload subsystem and implementation for pgwire credential changes #4168
Conversation
sklarsa
changed the title
sklarsa
changed the title
[PR Coverage check]😍 pass : 38 / 57 (66.67%) file detail
|
core/src/main/c/linux/filewatcher.c
Outdated
| struct file_watcher *fw = (struct file_watcher *)wp; | ||
|
|
||
| // Clean up the inotify watch descriptor first | ||
| free(fw->wd); |
There was a problem hiding this comment.
this call looks extremely wonky, we need to free the following:
- all mallocs, which pointers have been assigned to the struct
- all fds that have been assigned
core/src/main/c/linux/filewatcher.c
Outdated
|
|
||
| /* Re-copy the path so we can extract the filename */ | ||
| strcpy(pathCpy, fp); | ||
| char *filename = malloc(sizeof(char) * strlen(pathCpy) + 1); |
core/src/main/c/linux/filewatcher.c
Outdated
| struct file_watcher *fw; | ||
|
|
||
| /* Create the file descriptor for accessing the inotify API. */ | ||
| const int fd = inotify_init(); |
| @Override | ||
| public void close() { | ||
| if (!closed) { | ||
| reloadThread.interrupt(); |
There was a problem hiding this comment.
interrupt cannot be used to stop the thread, it is just a flag that is being set. Nor interrupt waits for the thread to stop. There should be a different mechanism, that guarantees thread is halted and also gurantees lack of race conditions.
I am wondering if closing fd would release the wait? After that the thread should wind down gracefully?
There was a problem hiding this comment.
Yes, this is a recent addition that I wasn't sure about. I'm removing the interrupt call, but still need to validate that all resources are properly released
core/src/main/c/linux/filewatcher.c
Outdated
| } | ||
| fw->fd = fd; | ||
| fw->wd = wd; | ||
| fw->filename = filename; |
There was a problem hiding this comment.
i don't think we need to hold on to the filename
There was a problem hiding this comment.
I use filename to match filesystem events so we only emit them for that specific file:
questdb/core/src/main/c/linux/filewatcher.c
Line 137 in 0e9807f
core/src/main/c/linux/filewatcher.c
Outdated
| inotify to watch the entire directory (and its contents) for changes | ||
| but only surface changes made to the watched file */ | ||
| char pathCpy[strlen(fp)]; | ||
| strcpy(pathCpy, fp); |
There was a problem hiding this comment.
we should not need a copy of string
There was a problem hiding this comment.
I found that when I didn't copy, the dirname call attempted to modify the fp string, which segfaulted because it is in protected memory. Per man pages:
Both dirname() and basename() may modify the contents of path, so it may be desirable to pass a copy when calling one of these functions.
https://linux.die.net/man/3/basename
I did find an extra, unnecessary strcpy which I deleted in 1264dac
| public InotifyFileEventNotifier(CharSequence dirPath) { | ||
| try (Path p = new Path()) { | ||
| p.of(dirPath).$(); | ||
| this.filewatcherPtr = setup(p.ptr()); |
There was a problem hiding this comment.
the setup returns 0 in case of parameter error, we should at least have an assert here
| import io.questdb.std.Misc; | ||
| import io.questdb.std.QuietCloseable; | ||
|
|
||
| public class FileWatcher implements QuietCloseable { |
There was a problem hiding this comment.
we need a test around file watcher, to make sure it can produce callbacks and can halt without segmentation fault
| private boolean closed; | ||
|
|
||
| public InotifyFileEventNotifier(CharSequence dirPath) { | ||
| try (Path p = new Path()) { |
There was a problem hiding this comment.
can be replaced with Path.getThreadLocal()
sklarsa
changed the title
…nto steve/hot-reload
|
@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 It looks like I was in the process of moving my The Bootstrap and ServerMain plumbing code is working as expected for the As for the cross-platform filewatcher stuff, here is where we are.
I'm happy to sync with you on this when you have time. I'd love to finally finish this thing. |
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