-
Notifications
You must be signed in to change notification settings - Fork 147
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
add HOT-RELOAD feature and tests #261
base: master
Are you sure you want to change the base?
Conversation
@apavanello Nice, this looks awesome. Just to clarify here - all you're enabling for Hot Reload is the queues and the topics themselves, correct? The other things, Log File/Region/QueueAttributes/etc. those look omitted from |
cc2c103
to
a43d9fd
Compare
@apavanello This change looks good, but it looks like it needs a rebase now. I have been playing with it today and it looks like this conflicts with some of the tests. I think they're treading on each other's memory addresses. Anyway - I have a branch locally with some refactoring. I'll push it when I can if you want to see - though it will be significant, not all particularly related to your code. Thought, if you could share your k8 use case that would be really helpful - I want to make sure whatever I come up with matches your needs too. In the meantime, if you want to rebase this and get it working that works too and I'll merge it. Up to you! Let me know! |
if !ok { | ||
return | ||
} | ||
if event.Has(fsnotify.Remove) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at this a bit deeper, I don't think I fully understand this block. Is it because in your environment - this "remove" event is what happens on a file change (in a "remove" then "recreate" pattern)? If so, the file name isn't changing is it? Could we not just do the wait/loop to check if it's rebuilt and then proceed down with the rest of the reloading calls?
I think I'm confused about 2 things really.
- Why call
StartWatcher
again, and synchronously? - Why block the outer function with the
quit
channel? In most of these non-k8 with the remove event cases would that ever actually hang up or is that goroutine just orphaned now?
Hello there. But I can tell you that in the case of |
Ah, I can totally understand that alternate project business, story of my life. 😀 I have a sizeable PR that I'll probably want to incorporate anyway, but I'll incorporate whatever you come up with too. Alternately you can review my code when it's ready. I'm fleshing out some tests but I think it's nearly ready. I'll play with the k8 use case to make sure I'm capturing it to. Thanks for the info! |
@apavanello So - I am throwing this out there as a new PR. The more I got into it, the more I realized it's a major feature. but my PR has your commits in there too. I want to do a few things on this since operating in this memory space is just hard. I would say - if you want to take yours over the line, go for it. I will rebase, if not, I'll do what I can. Also, if you have the bandwidth to keep an eye on it for your own use cases I'd appreciate it. |
#260