Skip to content

Fix bug with BigIntArray serialization#90142

Merged
elasticsearchmachine merged 3 commits intoelastic:mainfrom
nik9000:netty_int_array_bug_1
Sep 21, 2022
Merged

Fix bug with BigIntArray serialization#90142
elasticsearchmachine merged 3 commits intoelastic:mainfrom
nik9000:netty_int_array_bug_1

Conversation

@nik9000
Copy link
Copy Markdown
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. Please avoid if you can. v8.5.0 labels Sep 19, 2022
@elasticsearchmachine
Copy link
Copy Markdown
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
Copy Markdown
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

out.writeVInt(intSize * 4);
int lastPageEnd = intSize % INT_PAGE_SIZE;
if (lastPageEnd == 0) {
for (int i = 0; i < pages.length; i++) {
Copy link
Copy Markdown
Contributor

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
Copy Markdown
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-without-approval 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-without-approval 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. Please avoid if you can. >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