Change objstore Delete() contract to make it idempotent

[pracucci]

Is your proposal related to a problem?

The objstore Delete() is expected to return error if the object doesn't exist in the bucket:

thanos/pkg/objstore/objstore.go

Lines 44 to 46 in e9260e3

	// Delete removes the object with the given name.
	// If object does not exists in the moment of deletion, Delete should throw error.
	Delete(ctx context.Context, name string) error

This design decision has some drawbacks:

• S3 "delete object" API is idemponent and Thanos S3 client (based on Minio) is idempotent as well. Basically it violates the contract.
• We have some places where we check for existence before deleting (eg. block.Delete()), while if it would be idempotent we could skip the check for existence at all.
• Design wise, isn't more RESTful having deletes idempotent?

However, I may be missing the reason behind this design decision, so I've opened an issue to discuss it.

Describe the solution you'd like

I would propose to change objstore clients Delete() contract to make it idempotent. Object stores where delete is idempotent (eg. S3) would require no changes, while others would require handling the "not found" error status code (typically a 404, like on GCS).

[bwplotka]

I am almost certain that we did this ONLY to match with "some" obj storage APIs we were inspired by (GCS) . We are definitely ok to make it idempotent long term (:

Design wise, isn't more RESTful having deletes idempotent?

Maybe, but I kind of like explicitness. If you delete something that does not exists, I want to know that as a caller. This might mean some eventual consistency or race, so then caller can just ignore it. OR I really want to delete something and I want to know about deletion happening.

The simple case will be instrumentation. If we delete I want to make that we deleted something. If 20 writes deletes I just have info "they tried to delete and maybe they deleted and maybe it was already deleted". Such instrumentation/metric will NOT tell us how many deletes were in DB (e.g objstore), but rather how many attempts were there.

Apart of instrumentation, I don't see other use cases for non-idempotent Delete, but the instrumentation one might be important, not sure. Ideally, allowing caller to decide makes sense?

I think I would be happy to make it idempotent, our goal with objstore is indeed to reduce API calls.

[pracucci]

I think I would be happy to make it idempotent, our goal with objstore is indeed to reduce API calls.

I thought the rest of the comment was leading towards not making it idempotent. If it's idempotent, from the consumer perspective, then the Delete() would act the same exact way regardless the object existed or not in the storage.

[bwplotka]

Yea, because I am torn.

Just curious, the only benefit here is really API calls? Because in a PR for meta json, we could just try to delete and filter out NotFound error, no? To achieve this (:

[pracucci]

Because in a PR for meta json, we could just try to delete and filter out NotFound error, no? To achieve this (:

Yes, definitely. That's another valid approach, but I think the current implementation doesn't guarantee it. What I mean is: we should check this is true for all object store clients and, if so, document it in the Delete() contract.

The only downside I see (but not a big deal, if correctly documented) is that it's not possible to implement on S3 (where "delete object" API is idempotent) unless we issue an "head object" before deleting, which I would prefer to avoid (to reduce API calls).

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Closing for now as promised, let us know if you need this to be reopened! 🤗