mirror with --watch should copy and exit for s3 endpoints #2345
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2345 +/- ##
==========================================
- Coverage 10.76% 10.76% -0.01%
==========================================
Files 103 103
Lines 7721 7723 +2
==========================================
Hits 831 831
- Misses 6756 6758 +2
Partials 134 134
Continue to review full report at Codecov.
|
cmd/parallel-manager.go
Outdated
| fn := <-p.queueCh | ||
| if fn == nil { | ||
| fn, ok := <-p.queueCh | ||
| if fn == nil || !ok { |
There was a problem hiding this comment.
why are you using fn before checking ok?
There was a problem hiding this comment.
I can use only one..
cmd/mirror-main.go
Outdated
| scanBar: func(s string) {}, | ||
| statusCh: make(chan URLs), | ||
| queueCh: make(chan func() URLs), | ||
| wgStatus: new(sync.WaitGroup), |
There was a problem hiding this comment.
is watcherRunning member needed elsewhere or can it be removed from mirrorJob?
There was a problem hiding this comment.
It can be removed not needed.
harshavardhana
force-pushed
the
fix-mirror
branch
3 times, most recently
from
a042ca1 to
ef47270
Compare
|
@vadmeste PTAL |
|
@vadmeste can you take a look? |
cmd/mirror-main.go
Outdated
| if mj.isWatch { | ||
| <-mj.trapCh | ||
| mj.watchMirror(ctx, cancelMirror) |
There was a problem hiding this comment.
We need to check the error message returned by this error, and I guess only print it as a warning.
There was a problem hiding this comment.
Requires some restructuring let me do that.
cmd/mirror-main.go
Outdated
| // start the status go routine | ||
| mj.startStatus() | ||
|
|
||
| // Starts additional watcher thread for watching for new events. | ||
| if mj.isWatch { | ||
| go mj.watchMirror(ctx, cancelMirror) |
There was a problem hiding this comment.
It is not commented in the code, but the reason why we have this in a go-routine is to avoid missing new changes in source such as removing files, adding new files.. this is very useful especially when mirroring takes some times. Any reason why you are removing this @harshavardhana ?
There was a problem hiding this comment.
I know the reason @vadmeste , i generally thought it is not a big deal. Since they will be caught up later but i see now that it might be not a good idea and it is possible that we do not exit properly.
harshavardhana
force-pushed
the
fix-mirror
branch
from
ef47270 to
aecfc86
Compare
|
Simplified the code further @vadmeste - avoiding racy updates to provide struct members. Removed |
cmd/mirror-main.go
Outdated
| return | ||
| default: | ||
| errorIf(err, "Unexpected error during monitoring.") | ||
| errorIf(err.Trace(), "Watch is not implemented ignoring.") |
There was a problem hiding this comment.
I think this message is better, 'Unable to watch source path'.
Current code wouldn't exit unless we press CTRL+C even if the endpoints do not support --watch, this PR fixes this behavior.
harshavardhana
force-pushed
the
fix-mirror
branch
from
aecfc86 to
ce2f2ae
Compare
|
fixed @vadmeste PTAL |
Current code wouldn't exit unless we press CTRL+C
even if the endpoints do not support --watch, this PR
fixes this behavior.