Introduce new ObjectPart type

[passaro]

Description of change

The ObjectPart type represents a slice of the content of an S3 object. It contains the information required to identify the object it belongs to and its offset in it. It also maintains checksums to validate its integrity. The new type is used in the prefetcher and replaces both ChecksummedBytes and Part.

Does this change impact existing behavior?

No.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

[jamesbornholt]

Cool refactor. I kinda thought that we'd want to use ChecksummedBytes to protect other stuff like SSE keys, but maybe that's more trouble than it's worth?

[dannycjones]

I have took a pass over it and the change looks good overall. I put a few comments too, particularly important would be the one around if we want to keep ChecksummedBytes around and use it as an internal detail of ObjectPart.

To get this merged, it needs to be broken down a bit so we can review properly that everything is correct. Perhaps we can split it either into multiple commits in one PR which will be squashed, or just multiple PRs. Assuming we want to merge as is...

• Add ObjectId which is fairly simple and can be reviewed very quickly. Maybe some of the lines that moved around can be included in that.
• Introduce the new ObjectPart, and replace usages of Part only first? (the big change)
• Follow up with replacing usages of ChecksummedBytes in things like the data cache, and removing ChecksummedBytes

[passaro]

Rebased on main which now includes the ObjectId change. I've temporarily left ObjectPart in the checksums module to make it easier to review the changes compared to ChecksummedBytes.
I'll move it to the object module once reviewed.

[dannycjones]

Provided some comments. This change is still too large. How else can we break this down?

• Refactor ChecksummedBytes to adopt storing bytes and range, rather than a slice of orig_bytes
• Simple stuff like the renaming of ChecksummedBytes::new to ChecksummedBytes::new_from_inner_data, so the next change becomes adding offset only. Maybe also some of the test changes, where we just move things like constructing a Bytes to its own line. Simple to review, but noise in a risky PR like this.
• Rename current Part to ObjectPart, so a lot of the uses are again a simple rename PR to review.
• Then move the part logic into this module, moving the guard from part into this type.

When it's broken down like that (or similar), can we break down those commits at all?

Ultimately, we want to make this as easy as possible for the reviewer to look at and approve considering the code we're actually changing. Try and make the PRs as simple as possible so anyone can review and get these in, so we can complete this refactor in good time.

[dannycjones]

need to fix comment

Suggested change

	/// Computed over `part_id`, `offset`, `buffer_checksum`, and `range` (but not `buffer`).
	/// Computed over `object_id`, `offset`, `buffer_checksum`, and `range` (but not `buffer`).

Maybe we just remove this sentence, and instead point to the method that computes the checksum. Otherwise, we need to maintain this comment on top of that other method.

[dannycjones]

This should still be behind the guard requiring object ID and offset to be provided?