Remove the Snapshot class in favor of using SnapshotInfo

[abeyad]

o/e/snapshots/Snapshot and o/e/snapshots/SnapshotInfo contain the same
fields and represent the same information. Snapshot was used to
maintain snapshot information to the snapshot repository, while
SnapshotInfo was used to represent the snapshot information as presented
through the REST layer. This removes the Snapshot class and combines
all uses into the SnapshotInfo class.

Relates to #18156

[nik9000]

The same duality exists for tasks too. Task and TaskInfo. The difference there is that Task isn't Writeable or ToXContent but TaskInfo is. TaskInfo as a snapshot of a task. It works really well! I'm not sure if that was the intent here though. I'll review though.

[nik9000]

Collections.addAll?

[abeyad]

You're right, changing

[abeyad]

@nik9000 The thought for merging Snapshot with SnapshotInfo came about because @ywelsch and I were talking about naming SnapshotId and it got to the point where we wanted to rename the current SnapshotId to Snapshot and make SnapshotId composed of Snapshot plus a uuid. At that point, I noticed that Snapshot and SnapshotInfo represent the same exact info, the only difference really is that Snapshot's toXContent differed a bit (what was written to the repository) than SnapshotInfo, where the x-content is a bit more verbose. I thought it was easier to solve this by merging the two and just having two different methods for x-content (the verbose and the non-verbose one). We had a few places too where we would get a Snapshot, only to convert it to a SnapshotInfo for the APIs... this cleans up all that and the creation of unnecessary objects.

I can see the reverse point - that the two uses have different intents, but since I felt the intents only differed in how they are serialized for particular use cases, I went with this.

I'd appreciate your thoughts, if you think this makes sense

[nik9000]

While you are here why not remove the whole Fields class and just use strings? That is the "new style". For things that are used in more than one place we'll typically use constants - and if those things are using in parsing the constant will typically be a ParseField, but otherwise we've started to not make these Fields classes.

[abeyad]

++ will do

[nik9000]

I left some stylistic comments. I had a quick look around and I agree with you - these two classes look to be the same information just used in different contexts. I like the idea of throwing one away.

I don't think I need to do another review - either make the changes I asked for or reply explaining why they aren't a good idea so I'll learn something and merge when you ready. LGTM.

[nik9000]

I'd appreciate your thoughts,

I think that having the two classes makes sense for tasks because it is clear how they are different. They have subclasses that make them wildly different - compare BulkByScrollTask and BulkByScrollTask.Status. They are obviously different.

In this case these things are pretty much the same so I like merging them.

Also in my previous comment I wrote TaskInfo when I should have written TaskStatus. TaskStatus and Task are the dual classes.

[abeyad]

@nik9000 great, thank you for the review! I implemented all the changes you mentioned. The only one that is outstanding is that SnapshotInfo still implements FromXContentBuilder, which isn't ideal, but the ChecksumBlobStoreFormat depends on it.

[nik9000]

but the ChecksumBlobStoreFormat depends on it.

I can live with that! I'll take incremental progress over being bogged down in changes forever any day!

[abeyad]

@nik9000 great, thanks for the review!