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

add HOT-RELOAD feature and tests #261

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

Conversation

apavanello
Copy link

@Admiral-Piett
Copy link
Owner

@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 StartWatcher.

@Admiral-Piett
Copy link
Owner

@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) {
Copy link
Owner

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.

  1. Why call StartWatcher again, and synchronously?
  2. 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?

@apavanello
Copy link
Author

Hello there.
Sorry, I've been working on a new project at work for the past few months, and it's been taking up a lot of my time.
I need to look at the code again because I don't remember some of the details completely. I'll try to do it tonight when I'm done working.

But I can tell you that in the case of fsnotify.Remove, it's for if you want to use goaws in a Kubernetes environment. Kubernetes doesn't send clear signals of file changes or creation, it only sends a remove when the file state changes in a configmap.

@Admiral-Piett
Copy link
Owner

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!

@Admiral-Piett Admiral-Piett mentioned this pull request Nov 22, 2023
@Admiral-Piett
Copy link
Owner

@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.

#282

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

Successfully merging this pull request may close these issues.

2 participants