Skip to content

Arbitrary bytes in blob store register #96019

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

DaveCTurner
Copy link
Contributor

Today the blob store register supports recording only a long, represented as an 8-byte blob. We need to store a little more data in the register, so this commit generalises things to work with a BytesReference directly.

Today the blob store register supports recording only a `long`,
represented as an 8-byte blob. We need to store a little more data in
the register, so this commit generalises things to work with a
`BytesReference` directly.
@DaveCTurner DaveCTurner added >non-issue :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.9.0 labels May 11, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label May 11, 2023
Copy link
Contributor

@fcofdez fcofdez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I left a question regarding MAX_REGISTER_CONTENT_LENGTH


public class BlobContainerUtils {
private BlobContainerUtils() {
// no instances
}

public static final int MAX_REGISTER_CONTENT_LENGTH = Long.BYTES;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be greater than Long.BYTES?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was intending to do that in a follow-up, to avoid changing too many things here.

slices.add(new BytesArray(randomByteArrayOfLength(sliceLen)));
remaining -= sliceLen;
}
return CompositeBytesReference.of(slices.toArray(BytesReference[]::new));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should generate the other BytesReference variants too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is enough - it means that we make #iterator() yield a fairly random selection of slice sizes, which is where I suspect the most bugs lurk. We'll also occasionally return a BytesArray in the case that there's only one slice.

if (bytesReference.length() == 0) {
return 0L;
} else if (bytesReference.length() == Long.BYTES) {
try (var baos = new ByteArrayOutputStream(Long.BYTES)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have some library functions to deal with this directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently not, or at least I couldn't find any.

Copy link
Contributor

@fcofdez fcofdez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the clarifications 👍

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 15, 2023
@DaveCTurner
Copy link
Contributor Author

Well this is frustrating. https://gradle-enterprise.elastic.co/s/jd6eqec3nbvfe looks like a related failure, but I've been running that test in a loop for ages without being able to reproduce it.

@DaveCTurner
Copy link
Contributor Author

I have been running this test in a loop for 20+ hours without another failure. I'm going to ignore it in the name of progress.

@elasticsearchmachine elasticsearchmachine merged commit 350beea into elastic:main May 16, 2023
@DaveCTurner DaveCTurner deleted the 2023-05-05-bytes-register branch May 16, 2023 10:16
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request May 16, 2023
elasticsearchmachine pushed a commit that referenced this pull request May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >non-issue Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants