[Dumper] Refactor pipeline ingestor #175
Conversation
pkg/dump/pipeline/pipeline_test.go
Outdated
| "k8s.io/client-go/kubernetes/fake" | ||
| ) | ||
|
|
||
| func genK8sObjects() []runtime.Object { |
There was a problem hiding this comment.
This is the only one that is exported, is that intended?
pkg/dump/pipeline/pipeline.go
Outdated
| // Setting up the worker pool with multi-threading if possible | ||
| // enable for raw file writer | ||
| // disable for targz writer (not thread safe) | ||
| wp, err := worker.PoolFactory(workerNumber, 1) |
There was a problem hiding this comment.
is 1 intended for worker > 1?
Does this actually run in multiple threads if you put workerNumber to like 5 but with a capacity of 1?
(Also, I would suggest having that "1" put into a variable, because that's not easily understandable, something like poolCapacity or something maybe?)
There was a problem hiding this comment.
yes, this is the capacity of the queue. In our case we do not need any buffer. I will add variable to make it clearier.
There was a problem hiding this comment.
I'm still unclear on why we don't want any more buffer, but LGTM!
(I guess we will see on the trace how much concurrency we are really getting. In any case I would assume it's not a big deal :D)
There was a problem hiding this comment.
it was to avoid any concurrency issue. I guess I can try to test it in another PR to see if perf improve and does not introduce new bugs.
pkg/dump/pipeline/pipeline_faker.go
Outdated
There was a problem hiding this comment.
What's the reason on not putting this in pipeline_test.go or any other test package?
I don't believe it's used anywhere outside of this package right? (the k8s_api_faker.go, on the other hand, is)
There was a problem hiding this comment.
because PipelineLiveTest() is used in dump package. Therefore, it needs to be outside from the test package. Therefore, all the other function needs to be outside also.
Refactor the pipeline ingestor to include more unit tests.