Introduce TransportGetSnapshotsAction#GetSnapshotsOperation

[DaveCTurner]

This commit introduces a class to encapsulate each invocation of
TransportGetSnapshotsAction, avoiding the need to pass around a huge
list of parameters between all the methods that make up the
implementation.

[elasticsearchmachine]

Pinging @elastic/es-distributed (Team:Distributed)

[DaveCTurner]

For context, I was looking at this in relation to #104607 and #95345. They should be fairly simple to fix, except that this class is currently too convoluted to do so. There's loads of other things to clean up here too, incorrect/outdated comments, inconsistent and misleading naming, excessive copying of lists into other lists, but I'm isolating this one change first for ease of review.

[DiannaHohensee]

I looked at the high level structural changes, and I left a number of questions.

I'm not sure where you're headed with further refactoring after this, but my default inclinations are typically to separate network handling – parsing, verifying, validating, response construction – from implementation code that accesses internal server state. Obviously that's outside the scope of this change, but I'd like to better understand what the ideal state is, to ensure that's where we're moving.

[DiannaHohensee]

Could we move this into its own file? It seems too big/complex to be reasonable as an inner class.

[DaveCTurner]

We could, but then basically nothing would be left in TransportGetSnapshotsAction and we'd still have all that size and complexity in one place. And indeed one could argue it's kinda more complex as a top-level class because that means the reader has to worry about who else in the package might be calling it, whereas as a private inner class we can see all the callers right in front of us.

[DiannaHohensee]

After talking offline, I think this is more of a lack of strong API boundaries problem. Ideally there's an API that says a method does X, and callers can use it as needed. But the GetSnapshotsOperation namespace isn't really a logical piece, rather a namespace we're dumping everything into until we refactor more meaningful boundaries. For the time being.

Multiple callers means deduplicating code, which sounds like a good problem.

[DaveCTurner]

I will admit I don't have a precise end-goal in mind. Clearly this area needs improvement, and experience shows that introducing a method object is usually a good first step in cases like this. I have played around with some follow-up changes, consolidating some logic and breaking up other bits, and this change definitely makes those things less noisy and therefore hopefully easier to review.

[DaveCTurner]

my default inclinations are typically to separate network handling – parsing, verifying, validating, response construction – from implementation code that accesses internal server state.

Sorry if I'm missing something but I don't think we're doing any parsing/verifying/validating of network stuff here. Constructing a response (by calling methods on Repository) is the whole point of this API, I'm not sure we could get rid of that.

[DiannaHohensee]

LGTM. Spoke offline with David. Here's a summary:

David has a rough idea to ultimately refactor the logic to make more sense, maybe booting out some logic to a separate file (better unit testing of some inner mechanics, perhaps), or at least un-blobbing the logic from inside a meaningless class namespace into meaningful logical separation. He's pushing most of the code into an inner class to get rid of the method parameter noise and friction to further refactoring, and then eventually following up with method refactors to rearrange the code into more sensical pieces.

Ultimately trying to make some performance improvements (meaningful result pagination, and not fetching all the snapshot info when only a subset is returned) some day and needs the code to be manageable first.