Adding a sync() function to filetarget #293
Conversation
|
Re: config structs, I tend to put them alongside the structs that they configure, to prevent circular imports. The only alternative is having them all in one place, and that means everything tends to import the config package, which I don't like. |
|
Re: config structs, I tend to put them alongside the structs that they configure, to prevent circular imports. The only alternative is having them all in one place, and that means everything tends to import the config package, which I don't like. |
|
Given the complexity of the sync code, I'm coming round to the idea of removing the ionotify watcher and just relying on sync. WDYT? |
|
To move the TargetConfig struct back to the targets package we would have to move the ScrapeConfig out of the api package and put it somewhere else to avoid the circular import, thoughts on what a good place for it would be? |
|
Re: the ionotify watcher, I was playing around yesterday with how the tailer works, and when pointed at a new file it will start at the beginning and "catch up" so to speak, so the polling nature of the sync function should work just fine. I'll think about it a little more but at the moment I don't think the event driven ionotify watcher is gaining us anything except some additional complexity and I would support removing it. |
|
move the targetConfig struct to the targets package and move the Config struct to its own package, eg mv that should resolve address the circular dependency issues |
slim-bean
force-pushed
the
refactor-sync
branch
4 times, most recently
from
602651e to
28c76a0
Compare
slim-bean
changed the title
slim-bean
force-pushed
the
refactor-sync
branch
from
28c76a0 to
ffb65b3
Compare
|
I think I'd like to bring up removing the fsnotify code in another issue/PR and get this merged as it's already getting hard to review with the refactorings |
slim-bean
force-pushed
the
refactor-sync
branch
from
ffb65b3 to
d11080e
Compare
slim-bean
force-pushed
the
refactor-sync
branch
from
d11080e to
c51a180
Compare
|
Updated based on feedback, cleaned up the sync function and simplified as much as I could, also attempted to reuse code between the fsnotify watcher stuff so there shouldn't be any duplicated logic in there as well either. |
|
Sorry @slim-bean! Super nit picky. But its super close now. |
… any events missed by fsnotify
…osed in testing, we want to stop the tailer before shutting down our tailer loop
…config consistent
moved tailer to its own file refactoring filetarget to make synclogic easier to follow and reused as much as possible
slim-bean
force-pushed
the
refactor-sync
branch
from
8e42dca to
afb9036
Compare
| @@ -7,3 +7,4 @@ cmd/loki/loki | |||
| cmd/promtail/promtail | |||
| /loki | |||
| /promtail | |||
| .idea/ | |||
There was a problem hiding this comment.
Whats this? Probably belongs in ~/.gitignore and then do git config --global core.excludesfile $HOME/.gitignore
| @@ -27,8 +29,7 @@ func TestLongSyncDelayStillSavesCorrectPosition(t *testing.T) { | |||
|
|
|||
| err := os.MkdirAll(dirName, 0750) | |||
| if err != nil { | |||
| t.Error(err) | |||
| return | |||
| t.Fatal(err) | |||
There was a problem hiding this comment.
Should have mentioned this earlier, but I prefer require.NoError(t, err) instead of these 3 lines. We can come back and tidy this up.
This changes the forGivenIngestors function in pkg/querier/ingester_querier.go to use `DoUntilQuorum` (from [PR #293](grafana/dskit#293)) instead of `Do`. `DoUntilQuorum` is much more efficient than `Do`. The exact differences between them can be seen [here](grafana/dskit#293). --------- Co-authored-by: Trevor Whitney <trevorjwhitney@gmail.com>
Update from upstream repository
I still want to add a test to verify the sync function will remove files properly.
I ran into a circular imports issue when adding a config to the FileTarget, because the targets package depends on the api package for ScrapeConfig, so if I put the FileTarget config in the targets package it creates a circular import.
I think it made most sense to put the TargetConfig in the api package rather than move the ScrapeConfig to the target package. Though this leads me to think all the other configs (positions, client) should also be moved to the api package for consistency?
I'm also open to another solution if I'm missing something obvious