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

Ignore chmod events on UI config watcher. #2254

Merged
merged 4 commits into from
May 21, 2020
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions cmd/query/app/static_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,16 +149,17 @@ func (sH *StaticAssetsHandler) watch() {
}
Copy link
Member

@yurishkuro yurishkuro May 20, 2020

Choose a reason for hiding this comment

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

L140: could we please refactor this nested go func() into a top function? There are 5 levels of indentation/nesting here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I separated the go func() into another function. That removes some indentation levels.

continue
}
if event.Op&fsnotify.Write == fsnotify.Write || event.Op&fsnotify.Create == fsnotify.Create {
Copy link
Member

Choose a reason for hiding this comment

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

  1. reduce nesting by inverting the condition and calling continue, as above
  2. should this be checking event.Name to match the actual config file, rather than reacting to all changes in the directory?
  3. another option to reduce logging is to compare the file context before and after, and only log on changed content. This is the approach taken for reloading sampling strategies file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Ok, sounds reasonable.

  2. I agree , but at the same time I'm not sure why in https://github.com/jaegertracing/jaeger/pull/2254/files#diff-5893b37f4eb854b962cce2c3be922456R180 we watch directory changes too, that line gave me the impression that we want to react to directory changes , but not sure why and I might be misunderstanding something here.

  3. Compare file content, we can do that, or we can compute the md5sum or something like that. but Do we really need that? The only scenario which I observed the excessive logging is due CHMOD events, of course can be other scenarios and a comparison could prevent future issues with like this one.

Copy link
Member

Choose a reason for hiding this comment

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

2 - I remember something about renaming in k8s, etc. which required watching the directory. And when watching the directory, there could be many other changes happening, although it's still strange that the user experience logs every 0.5s. By checking that the event is actually for the file name we want we can eliminate all other events in the dir.

3 - we can try with just adding the name check, although doing a full comparison on (string)content is trivial, just a couple of lines right in this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did all except the content comparison, I don't think we need it at this point but if you think is good I can add it, as you said is a trivial thing to do.

// this will catch events for all files inside the same directory, which is OK if we don't have many changes
sH.options.Logger.Info("reloading UI config", zap.String("filename", sH.options.UIConfigPath))

// this will catch events for all files inside the same directory, which is OK if we don't have many changes
sH.options.Logger.Info("reloading UI config", zap.String("filename", sH.options.UIConfigPath))
content, err := loadIndexBytes(sH.assetsFS.Open, sH.options)
if err != nil {
sH.options.Logger.Error("error while reloading the UI config", zap.Error(err))
}

content, err := loadIndexBytes(sH.assetsFS.Open, sH.options)
if err != nil {
sH.options.Logger.Error("error while reloading the UI config", zap.Error(err))
sH.indexHTML.Store(content)
}

sH.indexHTML.Store(content)
case err, ok := <-watcher.Errors:
if !ok {
return
Expand Down