Add context handling to coosyncconf gather #383
Conversation
balanza
requested review from
CDimonaco and
arbulu89
and removed request for
CDimonaco
balanza
force-pushed
the
corosyncconf-context
branch
2 times, most recently
from
56ef014 to
c9f3bd2
Compare
balanza
force-pushed
the
corosyncconf-context
branch
from
c9f3bd2 to
24fb176
Compare
There was a problem hiding this comment.
Hey @balanza
I will play the devils advocate.
Which are the chances and consequences in time to get a cancellation request during this files reading? (considering the file might be around 50 lines).
Even if you catch that exact same point, how long this would block the cancellation?
Let's not forget that the propagation of the context is mostly to avoid a blocked state during on this, more than error reporting (if we are going to cancel, we don't care that much about the error).
In the commands and other operations had more sense, as we can have long going command executions, but reading files, which most of the cases are small.
I have the feeling that this is as overcomplicated solution for a problem we don't have.
What do you think?
|
|
||
| for fileScanner.Scan() { | ||
| fileLines = append(fileLines, fileScanner.Text()) | ||
| if ctx.Err() != nil { |
There was a problem hiding this comment.
Do we need to check this?
I mean, utils.ScanFileContextis already returning an error in this case.
I wouldn't worry to check this thinking that between the scan file and this small piece of code we could have the cancellation.
If we are going to do so in all the gatherers (I think it is an overkill) I think we should do it in the gathering.go code.
There we already have a switch after running the Gather, so we could check there.
The purpose of thie context is to not block rather than notifying an error
| @@ -212,7 +212,7 @@ func (suite *CorosyncConfTestSuite) TestCorosyncConfFileNotExists() { | |||
| factsGathered, err := c.Gather(context.Background(), factsRequest) | |||
|
|
|||
| expectedError := &entities.FactGatheringError{ | |||
| Message: "error reading corosync.conf file: could not open corosync.conf file: " + | |||
| Message: "error reading corosync.conf file: could not open not_found: " + | |||
There was a problem hiding this comment.
nitpick: Just with this output, it looks like we are not piping properly the error messages
| errChan := make(chan error, 1) | ||
| contentChan := make(chan []string) | ||
|
|
||
| go func() { |
There was a problem hiding this comment.
question: Why do we need a thread here?
Having the for and select in the main thread is not enough?
At the end, you are staring the thread and waiting right away blocking the function. Why not simply return from the function with the error?
Or If you need to break the for loop you could add a simple variable that is set to true when the context is done
|
@arbulu89 I have the same feeling. I made the PR also to raise any concerns about whether it's needed or not. I think we can get rid of the code and keep the test, to ensure the interfaces are confirmed. What do you think? |
I'm fine with that. |
Following #376
In addition, two helper functions are added to deal with channel and file reads