This error is already passed to the caller of this function. Do we need to log it also, since it is to the caller to decide what to do with it and the error is already logged.

If we don't log it here, sometimes it is difficult to know which RPC call actually failed. This is a problem with bigger files, when you reach size of frame size and various limitations.

We can add more descriptive error messages where this function is called and error logged. There should not be the same error in two log lines.

You do have the line number when this is logged, so you know which one actually emitted it.

The point is that whenever trackChunks fails (which doesn't have an effect on the smoke tests, but has an effect on your debugging efforts), you want to know exactly where and why it failed while reviewing the logs.

Bottom line - you are right that we might log the same error twice, but we don't really care about that level of detail, as long as we have enough information in the logs to determine what happened.

I would be wrong if I say that I do not care about logging the same error twice. This is something that I consider a bad practice. If that is needed, then there is something wrong with our error propagation. I do not think that skipping on this principle contributes to the code cleanup and better architecture that we want to do.

Logging is very important, but it also should be correct, as someone can question why the same error happened twice during debugging. Or why the error needs double logging. If you really feel that all existing log lines are required, then that needs to be explained with comments to avoid that someone removes some of them in the future, breaking your expectations.

removed the log line.

Anton, please, do not think that I want to make you do something like "remove double logging, to make it more difficult to find where this error is coming from" in your latest commit 476c30c.

I also want to have cleaner code and for that we should agree on some principles. My opinion is that the function caller should decide if it should log the error or pass it annotated to its caller. But, not both as it may result in double logging which can be confusing. By skipping on this types of things we will just end up with more confusion in the code. That is why I asked at least for the comment.

I would also like to opinions from others @frncmx @justelad. If the general understanding is that this is a non-issue I am happy to accept it.

@janos you are missing the forest for the trees. We have more than 10 comments about a single log line in the swarm-smoke binary, the output of which only developers read. We are spending way too much time on the wrong thing.

The lifetime of this code will probably be short, since we will want to add much more functionality once we get to the point where the actual bugs in the swarm are addressed. Whether we log something once or twice in this test doesn't matter so much.

I really don't want to continue to have a discussion about a single log line, or whether this function has the best possible design - it is serving its purpose for the time being, and if it stops doing that in the near future - we will remove it and write a new one. This is most probably what's going to happen anyway, because tracing syncing chunk by chunk scales only so much - once we start running tests with bigger files, this won't be feasible in its current form.

I'd much rather we focus on why we have hundreds of lines of dead code and unit tests along them in the codebase, rather than whether we log an error once or twice.