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

Issue #1970: Ensure getStickyReadBookieIndex returns valid bookie index #2111

Merged
merged 1 commit into from
Jun 21, 2019

Conversation

sijie
Copy link
Member

@sijie sijie commented Jun 13, 2019

Descriptions of the changes in this PR:

Master Issue: #1970
Related Issues: apache/pulsar#3715 apache/pulsar#4526

Motivation

Fixes #1970

By default bookie uses a random generator to generate a bookie index as the sticky
read bookie index. However the random generator will generate negative numbers. Hence
it will generate negative bookie indexes in write set and cause ArrayIndexOutOfBoundException
when bookkeeper attempts to read entries.

Modifications

Make sure getStickyReadBookieIndex not return negative number.

Verify this change

This problem introduced by a random generator. It is very hard to write a unit test to reproduce this issue.
Existing StickyRead tests are good to cover this code change.


In order to uphold a high standard for quality for code contributions, Apache BookKeeper runs various precommit
checks for pull requests. A pull request can only be merged when it passes precommit checks. However running all
the precommit checks can take a long time, some trivial changes don't need to run all the precommit checks. You
can check following list to skip the tests that don't need to run for your pull request. Leave them unchecked if
you are not sure, committers will help you:

  • [skip bookkeeper-server bookie tests]: skip testing org.apache.bookkeeper.bookie in bookkeeper-server module.
  • [skip bookkeeper-server client tests]: skip testing org.apache.bookkeeper.client in bookkeeper-server module.
  • [skip bookkeeper-server replication tests]: skip testing org.apache.bookkeeper.replication in bookkeeper-server module.
  • [skip bookkeeper-server tls tests]: skip testing org.apache.bookkeeper.tls in bookkeeper-server module.
  • [skip bookkeeper-server remaining tests]: skip testing all other tests in bookkeeper-server module.
  • [skip integration tests]: skip docker based integration tests. if you make java code changes, you shouldn't skip integration tests.
  • [skip build java8]: skip build on java8. ONLY skip this when ONLY changing files under documentation under site.
  • [skip build java11]: skip build on java11. ONLY skip this when ONLY changing files under documentation under site.


Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

If this PR is a BookKeeper Proposal (BP):

  • Make sure the PR title is formatted like:
    <BP-#>: Description of bookkeeper proposal
    e.g. BP-1: 64 bits ledger is support
  • Attach the master issue link in the description of this PR.
  • Attach the google doc link if the BP is written in Google Doc.

Otherwise:

  • Make sure the PR title is formatted like:
    <Issue #>: Description of pull request
    e.g. Issue 123: Description ...
  • Make sure tests pass via mvn clean apache-rat:check install spotbugs:check.
  • Replace <Issue #> in the title with the actual Issue number.

Master Issue: apache#1970

*Motivation*

Fixes apache#1970

By default bookie uses a random generator to generate a bookie index as the sticky
read bookie index. However the random generator will generate negative numbers. Hence
it will generate negative bookie indexes in write set and cause ArrayIndexOutOfBoundException
when bookkeeper attempts to read entries.

*Modifications*

Make sure getStickyReadBookieIndex not return negative number.

*Verify this change*

This problem introduced by a random generator. It is very hard to write a unit test to reproduce this issue.
Existing StickyRead tests are good to cover this code change.
@sijie sijie changed the title Ensure getStickyReadBookieIndex returns valid bookie index Issue #1970: Ensure getStickyReadBookieIndex returns valid bookie index Jun 13, 2019
Copy link
Member

@jiazhai jiazhai left a comment

Choose a reason for hiding this comment

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

nice catch

@sijie
Copy link
Member Author

sijie commented Jun 14, 2019

run bookkeeper-server bookie tests

@sijie sijie added this to the 4.10.0 milestone Jun 21, 2019
@sijie sijie self-assigned this Jun 21, 2019
@sijie sijie merged commit 96fc2e2 into apache:master Jun 21, 2019
@sijie sijie deleted the fix_sticky_read branch June 21, 2019 03:20
sijie added a commit that referenced this pull request Jun 21, 2019
Descriptions of the changes in this PR:

Master Issue: #1970
Related Issues: apache/pulsar#3715 apache/pulsar#4526

*Motivation*

Fixes #1970

By default bookie uses a random generator to generate a bookie index as the sticky
read bookie index. However the random generator will generate negative numbers. Hence
it will generate negative bookie indexes in write set and cause ArrayIndexOutOfBoundException
when bookkeeper attempts to read entries.

*Modifications*

Make sure getStickyReadBookieIndex not return negative number.

*Verify this change*

This problem introduced by a random generator. It is very hard to write a unit test to reproduce this issue.
Existing StickyRead tests are good to cover this code change.




Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Jia Zhai <zhaijia@apache.org>, Yong Zhang <zhangyong1025.zy@gmail.com>

This closes #2111 from sijie/fix_sticky_read, closes #1970
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception when sending read requests
4 participants