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.

Sign up for GitHub

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

Jump to bottom

[TIEREDSTORAGE] Only seek when reading unexpected entry #5356

Merged
merged 3 commits into from
Oct 30, 2019
Merged

[TIEREDSTORAGE] Only seek when reading unexpected entry #5356

merged 3 commits into from
Oct 30, 2019

Conversation

ivankelly
Copy link
Contributor

The normal pattern from reading from an offloaded ledger is that the
reader will read the ledger sequentially from start to end. This means
that once a user reads an entry, we should expect that the next entry
they read will be the next entry in the ledger.

The initial implementation of the BlobStoreBackedReadHandleImpl (and
the S3 variant that preceeded it) didn't take this into
account. Instead it did a lookup in the index each time, to find the
block that contained the entry, and then read forward in the block
until it found the entry requested. This is fine for the first few
entries in the block, not so much for the last.

This PR changes the read behaviour to only seek if entryId read
from the block is either:

  • greater than the entry we were expecting to read, in which case we
    need to seek backwards in the block.
  • less than the entry expected, but also belonging to a different
    block to the expected entry, in which case we need to seek to the
    correct block.

This change improves read performance significantly. Adhoc benchmarks
shows that we can read from offloaded topics at ~160MB/s whereas
previously we could only manage <10MB/s.

@ivankelly
[TIEREDSTORAGE] Only seek when reading unexpected entry
f986d57 
The normal pattern from reading from an offloaded ledger is that the
reader will read the ledger sequentially from start to end. This means
that once a user reads an entry, we should expect that the next entry
they read will be the next entry in the ledger.

The initial implementation of the BlobStoreBackedReadHandleImpl (and
the S3 variant that preceeded it) didn't take this into
account. Instead it did a lookup in the index each time, to find the
block that contained the entry, and then read forward in the block
until it found the entry requested. This is fine for the first few
entries in the block, not so much for the last.

This PR changes the read behaviour to only seek if entryId read
from the block is either:
- greater than the entry we were expecting to read, in which case we
  need to seek backwards in the block.
- less than the entry expected, but also belonging to a different
  block to the expected entry, in which case we need to seek to the
  correct block.

This change improves read performance significantly. Adhoc benchmarks
shows that we can read from offloaded topics at ~160MB/s whereas
previously we could only manage <10MB/s.
@ivankelly ivankelly added type/bug The PR fixed a bug or issue reported a bug area/tieredstorage labels Oct 10, 2019
@ivankelly ivankelly self-assigned this Oct 10, 2019
sijie
sijie requested changes Oct 10, 2019
View reviewed changes
...n/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreBackedInputStreamImpl.java Outdated
@@ -116,7 +116,7 @@ public int read(byte[] b, int off, int len) throws IOException {

@Override
public void seek(long position) {
log.debug("Seeking to {} on {}/{}, current position {}", position, bucket, key, cursor);
log.info("Seeking to {} on {}/{}, current position {} (bufStart:{}, bufEnd:{})", position, bucket, key, cursor, bufferOffsetStart, bufferOffsetEnd);
Copy link
Member

Choose a reason for hiding this comment

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

Can this be annoying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, should be debug

Copy link
Member

Choose a reason for hiding this comment

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

@ivankelly can you change it to debug please?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ivankelly This is a very important change we are rooting for in 2.4.2. Merging this would be great.

@merlimat merlimat added this to the 2.4.2 milestone Oct 10, 2019
@merlimat merlimat mentioned this pull request Oct 10, 2019
Closed
@aahmed-se
Copy link
Contributor

@ivankelly can you address the comments

@sijie
Copy link
Member

sijie commented Oct 24, 2019

I reverted info logging to debug logging. once it passes CI, it is ready to merge.

@wolfstudy
Copy link
Member

run cpp tests

@wolfstudy wolfstudy merged commit 43bc790 into apache:master Oct 30, 2019
jerrypeng pushed a commit to jerrypeng/incubator-pulsar that referenced this pull request Dec 17, 2019
@ivankelly @merlimat
[TIEREDSTORAGE] Only seek when reading unexpected entry (apache#5356)
9f7e58d 
* [TIEREDSTORAGE] Only seek when reading unexpected entry

The normal pattern from reading from an offloaded ledger is that the
reader will read the ledger sequentially from start to end. This means
that once a user reads an entry, we should expect that the next entry
they read will be the next entry in the ledger.

The initial implementation of the BlobStoreBackedReadHandleImpl (and
the S3 variant that preceeded it) didn't take this into
account. Instead it did a lookup in the index each time, to find the
block that contained the entry, and then read forward in the block
until it found the entry requested. This is fine for the first few
entries in the block, not so much for the last.

This PR changes the read behaviour to only seek if entryId read
from the block is either:
- greater than the entry we were expecting to read, in which case we
  need to seek backwards in the block.
- less than the entry expected, but also belonging to a different
  block to the expected entry, in which case we need to seek to the
  correct block.

This change improves read performance significantly. Adhoc benchmarks
shows that we can read from offloaded topics at ~160MB/s whereas
previously we could only manage <10MB/s.

* Revert it back to debug
gaoran10 added a commit to gaoran10/pulsar that referenced this pull request Nov 19, 2020
@gaoran10
repair code for PR6335, affected PRs as below.
87f8b1e 
1. PR 4196 (2019/5/29 Merli)
Configure static PulsarByteBufAllocator to handle OOM errors (apache#4196)

2. PR 5356 (2019/10/30 Kelly)
[TIEREDSTORAGE] Only seek when reading unexpected entry (apache#5356)

3. PR 4433 (2019/6/4 Higham)
[tiered-storage] Add support for AWS instance and role creds (apache#4433)
@gaoran10 gaoran10 mentioned this pull request Nov 19, 2020
Merged
gaoran10 added a commit to gaoran10/pulsar that referenced this pull request Nov 20, 2020
@gaoran10
repair code for PR6335, affected PRs as below.
75f062f 
1. PR 4196 (2019/5/29 Merli)
Configure static PulsarByteBufAllocator to handle OOM errors (apache#4196)

2. PR 5356 (2019/10/30 Kelly)
[TIEREDSTORAGE] Only seek when reading unexpected entry (apache#5356)

3. PR 4433 (2019/6/4 Higham)
[tiered-storage] Add support for AWS instance and role creds (apache#4433)
codelipenghui pushed a commit that referenced this pull request Nov 21, 2020
@gaoran10
[Tiered Storage] Fix merge conflicts introduced by PR #6335 (#8630)
68759ff 
# Motivation

The PR #6335 lost some PR changes, related PRs as below.

1. PR 4196 (2019/5/29 Merli)
Configure static PulsarByteBufAllocator to handle OOM errors (#4196)

2. PR 5356 (2019/10/30 Kelly)
[TIEREDSTORAGE] Only seek when reading unexpected entry (#5356)

3. PR 4433 (2019/6/4 Higham)
[tiered-storage] Add support for AWS instance and role creds (#4433)
codelipenghui pushed a commit that referenced this pull request Nov 21, 2020
@gaoran10 @codelipenghui
[Tiered Storage] Fix merge conflicts introduced by PR #6335 (#8630)
8793963 
# Motivation

The PR #6335 lost some PR changes, related PRs as below.

1. PR 4196 (2019/5/29 Merli)
Configure static PulsarByteBufAllocator to handle OOM errors (#4196)

2. PR 5356 (2019/10/30 Kelly)
[TIEREDSTORAGE] Only seek when reading unexpected entry (#5356)

3. PR 4433 (2019/6/4 Higham)
[tiered-storage] Add support for AWS instance and role creds (#4433)

(cherry picked from commit 68759ff)
codelipenghui referenced this pull request in streamnative/pulsar-archived Nov 21, 2020
@gaoran10 @codelipenghui
[Tiered Storage] Fix merge conflicts introduced by PR apache#6335 (ap…
0388a1a 
…ache#8630)

# Motivation

The PR apache#6335 lost some PR changes, related PRs as below.

1. PR 4196 (2019/5/29 Merli)
Configure static PulsarByteBufAllocator to handle OOM errors (#4196)

2. PR 5356 (2019/10/30 Kelly)
[TIEREDSTORAGE] Only seek when reading unexpected entry (#5356)

3. PR 4433 (2019/6/4 Higham)
[tiered-storage] Add support for AWS instance and role creds (#4433)

(cherry picked from commit 68759ff)
(cherry picked from commit 8793963)
codelipenghui referenced this pull request in streamnative/pulsar-archived Nov 21, 2020
@gaoran10 @codelipenghui
[Tiered Storage] Fix merge conflicts introduced by PR apache#6335 (ap…
b3e35b1 
…ache#8630)

# Motivation

The PR apache#6335 lost some PR changes, related PRs as below.

1. PR 4196 (2019/5/29 Merli)
Configure static PulsarByteBufAllocator to handle OOM errors (#4196)

2. PR 5356 (2019/10/30 Kelly)
[TIEREDSTORAGE] Only seek when reading unexpected entry (#5356)

3. PR 4433 (2019/6/4 Higham)
[tiered-storage] Add support for AWS instance and role creds (#4433)

(cherry picked from commit 68759ff)
(cherry picked from commit 8793963)
(cherry picked from commit 0388a1a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skyrocknroll skyrocknroll skyrocknroll left review comments

@merlimat merlimat merlimat approved these changes

@sijie sijie sijie approved these changes

@jiazhai jiazhai Awaiting requested review from jiazhai

Assignees

@ivankelly ivankelly

Labels
area/tieredstorage type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
2.4.2
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ivankelly @aahmed-se @sijie @wolfstudy @merlimat @skyrocknroll