[core] Simplify OnlineFileSource

[jfirebaugh]

Sorry for the large'ish PR -- I reached the end point of where I could do refactors in parallel rather than sequentially, and I wanted to queue up reviews for all of these commits. Each commit is self-contained, if you want to go step by step. Or skip the diff and read the end result of online_file_source.cpp.

The end result is that it's easier to see the big picture of an OnlineFileRequestImpl's lifetime -- it starts with an optional cache request, then moves on to making periodic network requests, either for error retries or when the resource becomes stale.

From here, we can better judge if #2826 makes sense, embark on further refactors as suggested in #3454 (comment) and #3477 (comment), and begin to see how offline fits in.

cc @tmpsantos @kkaefer

[kkaefer]

Reviewed this PR; the overarching question is whether it makes sense to inline all of these methods; IMHO having them separate improves readability of the code.

[jfirebaugh]

Single-use methods are a trade off: you gain some benefit from shorter methods and explicit naming (if the names are good -- misleading names are often worse than no names), but it's more difficult to trace control and data flow through the code. Anyone wanting to understand or make changes to the separate method must locate the places it is called, understand the expectations of each calling context, and assure themselves they didn't miss any.

Typically I extract single-use methods only when I can clearly establish (and document and test) a method contract, or gain other benefits such as reducing the coupling of the extracted code. For example, extracting member code to a pure, static, non-member function that takes all its input as arguments and computes a return value.

When I started on this, the method factoring made the lifetime of a request difficult to follow. It was hard to determine basics like "can the control flow ever result in a cache get operation being retried"? I started inlining so that I could follow the data flow more easily and identify subsequent refactors. I believe that if you compare the before and after, the improvement is clear.

For getRetryTimeout specifically, I was hoping that after inlining there were some cases where we could take advantage of early exits to simplify the control flow. That didn't turn out to be the case however, so I'll go ahead and restore a separate function for calculating the timeout.

I stand by establishing scheduleCacheRequest and scheduleRealRequest as the sole two primary units of work. I think that makes the code substantially easier to understand.

[kkaefer]

Thanks for the clarification.

[jfirebaugh]

Merged.