Restore OPAQUE_MARKERS handling for list objects V1 #569
Conversation
We need this to support V1 clients that use the last object name in the response as a marker for the next page (which is allowed according to the S3 docs).
|
I'm concerned that s3-tests isn't exercising this behavior. Can you look at these and see what's missing? |
|
Do we have automated testing against a real azure blob store? |
|
@gaul Could you give me some pointers to where this should be tested? |
|
The ideal way to test this is https://github.com/ceph/s3-tests which I periodically rebase onto. But I don't have any automated integration tests against Azure presently. Alternatively maybe we could parameterize some S3Proxy listObjects tests to toggle |
|
@larshagencognite could you rebase this change? |
| @@ -1490,8 +1490,12 @@ private void handleBlobList(HttpServletRequest request, | |||
| if (Quirks.OPAQUE_MARKERS.contains(blobStoreType)) { | |||
| StorageMetadata sm = Streams.findLast(set.stream()).orElse(null); | |||
| if (sm != null) { | |||
| lastKeyToMarker.put(Maps.immutableEntry(containerName, | |||
| encodeBlob(encodingType, nextMarker)), nextMarker); | |||
| // TODO: verify if we need this handling at all for V2 | |||
|
@Decard6 any thoughts here? |
| lastKeyToMarker.put(Maps.immutableEntry(containerName, | ||
| encodeBlob(encodingType, nextMarker)), nextMarker); | ||
| // TODO: verify if we need this handling at all for V2 | ||
| String lastKey = isListV2 ? encodeBlob(encodingType, nextMarker) : sm.getName(); |
There was a problem hiding this comment.
@gaul
As long as isListV2 works as intended it should be good. Regardless I'd want to test the changes.
There was a problem hiding this comment.
All good. The change doesn't break pagination with V2.
|
What do you need to test (I can) to merge this? I confirmed that v1 list works after this change as I was having issues. |
|
It would be great if there were some kind of test that exercises this condition so I can confidently merge this PR. Could you submit a test that fails before this PR and succeeds with it? Note that CI includes the Azurite storage emulator so this should be easy. |
We need this to support V1 clients that use the last object name in the response as a marker for the next page (which is allowed according to the S3 docs).