Skip to content
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

Fix bug with BigIntArray serialization #90142

Merged
merged 3 commits into from
Sep 21, 2022

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Sep 19, 2022

This fixed a bug with the BigIntArray serialization that I wrote in #89668 where it'd skip the entire final block if we used all of it.

@nik9000 nik9000 added >non-issue :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. v8.5.0 labels Sep 19, 2022
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team label Sep 19, 2022
This fixed a bug with the `BigIntArray` serialization that I wrote in elastic#89668
where it'd skip the entire final block if we used all of it.
Copy link
Member

@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

@@ -54,11 +54,17 @@ public void writeTo(StreamOutput out) throws IOException {
}
int intSize = (int) size;
out.writeVInt(intSize * 4);
int lastPageEnd = intSize % INT_PAGE_SIZE;
if (lastPageEnd == 0) {
for (int i = 0; i < pages.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

NIT: I guess you could also do for (page : pages) { here?

Copy link
Member

@not-napoleon not-napoleon left a comment

Choose a reason for hiding this comment

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

LGTM

@nik9000 nik9000 added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 21, 2022
@elasticsearchmachine elasticsearchmachine merged commit 1684ab7 into elastic:main Sep 21, 2022
@nik9000 nik9000 deleted the netty_int_array_bug_1 branch September 21, 2022 14:21
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Sep 22, 2022
* main: (186 commits)
  [DOCS] Add 8.5 release notes and fix links (elastic#90201)
  Mute DownsampleActionSingleNodeTests.testCannotRollupWhileOtherRollupInProgress (elastic#90198)
  Bump to version 8.6.0
  Increase the minimum size of the management pool to 2 (elastic#90193)
  Speed `getIntLE` from `BytesReference` (elastic#90147)
  Restrict nodes for testClusterPrimariesActive1 (elastic#90191)
  Fix bug with `BigIntArray` serialization (elastic#90142)
  Synthetic _source: test _source filtering (elastic#90138)
  Modernize cardinality agg tests (elastic#90114)
  Mute failing test (elastic#90186)
  Move assertion in ES85BloomFilterPostingsFormat to fix test (elastic#90150)
  Restrict nodes for testClusterPrimariesActive2 (elastic#90184)
  Batch index delete cluster state updates (elastic#90033)
  Register stable plugins in ActionModule (elastic#90067)
  Mute failing test (elastic#90180)
  [HealthAPI] Disk: Use _ for diagnosis id (elastic#90179)
  [HealtAPI] Disk: use shorter help URLs (elastic#90178)
  Fixing disk health indicator unit tests (elastic#90175)
  Enable the health node and the disk health indicator elastic#84811 (elastic#90085)
  Add missing Disk Indicator health api IDs (elastic#90174)
  ...
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Sep 22, 2022
* main: (121 commits)
  [DOCS] Add 8.5 release notes and fix links (elastic#90201)
  Mute DownsampleActionSingleNodeTests.testCannotRollupWhileOtherRollupInProgress (elastic#90198)
  Bump to version 8.6.0
  Increase the minimum size of the management pool to 2 (elastic#90193)
  Speed `getIntLE` from `BytesReference` (elastic#90147)
  Restrict nodes for testClusterPrimariesActive1 (elastic#90191)
  Fix bug with `BigIntArray` serialization (elastic#90142)
  Synthetic _source: test _source filtering (elastic#90138)
  Modernize cardinality agg tests (elastic#90114)
  Mute failing test (elastic#90186)
  Move assertion in ES85BloomFilterPostingsFormat to fix test (elastic#90150)
  Restrict nodes for testClusterPrimariesActive2 (elastic#90184)
  Batch index delete cluster state updates (elastic#90033)
  Register stable plugins in ActionModule (elastic#90067)
  Mute failing test (elastic#90180)
  [HealthAPI] Disk: Use _ for diagnosis id (elastic#90179)
  [HealtAPI] Disk: use shorter help URLs (elastic#90178)
  Fixing disk health indicator unit tests (elastic#90175)
  Enable the health node and the disk health indicator elastic#84811 (elastic#90085)
  Add missing Disk Indicator health api IDs (elastic#90174)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. >non-issue Team:Distributed Meta label for distributed team v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants