Skip to content

Conversation

@fcofdez
Copy link
Contributor

@fcofdez fcofdez commented Oct 6, 2021

Today we are using Math.ceil in order to calculate the number of
chunks in the request. Since we cast the values to a double...
there be dragons. This could cause issues depending on the platform.

This commit uses good old integers to compute the number of parts.

Today we were using Math.ceil in order to calculate the number of
chunks in the request. Since we cast the values to a double...
there be dragons. This could cause issues depending on the platform.

This commit uses good old integers to compute the number of parts.
@fcofdez fcofdez added >bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v7.16.0 v7.15.1 labels Oct 6, 2021
@elasticmachine
Copy link
Collaborator

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

Copy link
Contributor

@original-brownbear original-brownbear 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 Francisco!

// by 64 elements), that's why we provide 64kb buffers.
return Flux.range(0, (int) Math.ceil((double) length / (double) chunkSize))

// length it's at most 100MB so it's safe to cast back to an integer in this case
Copy link
Contributor

@original-brownbear original-brownbear Oct 6, 2021

Choose a reason for hiding this comment

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

length is I guess:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦

@fcofdez fcofdez added the auto-backport Automatically create backport pull requests when merged label Oct 6, 2021
@fcofdez fcofdez merged commit c766b5f into elastic:master Oct 6, 2021
@fcofdez
Copy link
Contributor Author

fcofdez commented Oct 6, 2021

Thanks Armin!

fcofdez added a commit to fcofdez/elasticsearch that referenced this pull request Oct 6, 2021
…stic#78772)

Today we are using Math.ceil in order to calculate the number of
chunks in the request. Since we cast the values to a double...
there be dragons. This could cause issues depending on the platform.

This commit uses good old integers to compute the number of parts.
fcofdez added a commit to fcofdez/elasticsearch that referenced this pull request Oct 6, 2021
…stic#78772)

Today we are using Math.ceil in order to calculate the number of
chunks in the request. Since we cast the values to a double...
there be dragons. This could cause issues depending on the platform.

This commit uses good old integers to compute the number of parts.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
7.x
7.15

fcofdez added a commit that referenced this pull request Oct 7, 2021
…ws (#78775)

Today we are using Math.ceil in order to calculate the number of
chunks in the request. Since we cast the values to a double...
there be dragons. This could cause issues depending on the platform.

This commit uses good old integers to compute the number of parts.

Backport of #78772
fcofdez added a commit that referenced this pull request Oct 7, 2021
#78774)

Today we are using Math.ceil in order to calculate the number of
chunks in the request. Since we cast the values to a double...
there be dragons. This could cause issues depending on the platform.

This commit uses good old integers to compute the number of parts.

Backport of #78772
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged >bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v7.15.1 v7.16.0 v8.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants