Skip to content

WIP: [Issue 218] Add Seek functions to Reader - #222

Closed
cornelk wants to merge 1 commit into
apache:masterfrom
cornelk:add_reader_seek
Closed

WIP: [Issue 218] Add Seek functions to Reader#222
cornelk wants to merge 1 commit into
apache:masterfrom
cornelk:add_reader_seek

Conversation

@cornelk

@cornelk cornelk commented Apr 14, 2020

Copy link
Copy Markdown

Fixes #218

WIP due to TestReaderSeek hanging when executing reader.Next() after reader.Seek() - not sure why this happens and how to continue here, any help or hint is welcome.

Motivation

Seek() was missing in Reader interface but is available in the C++ client lib.

Modifications

  • implemented Seek in the similar way as Consumer does

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • TestReaderSeek*

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): ( no)
  • The public API: (yes)
  • The schema: (don't know)
  • The default values of configurations: (no)
  • The wire protocol: (no)

Documentation

  • Does this pull request introduce a new feature? (yes)
  • If yes, how is the feature documented? (GoDocs)

@wolfstudy

Copy link
Copy Markdown
Member
panic: test timed out after 20m0s

@wolfstudy wolfstudy added this to the 0.1.1 milestone May 15, 2020
@wolfstudy
wolfstudy requested review from merlimat and wolfstudy May 15, 2020 05:23

@wolfstudy wolfstudy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM +1

@wolfstudy wolfstudy closed this May 15, 2020
@wolfstudy wolfstudy reopened this May 15, 2020
@wolfstudy wolfstudy modified the milestones: 0.1.1, 0.2.0 Jun 9, 2020
@wolfstudy

Copy link
Copy Markdown
Member

Move this change to 0.2.0

Comment thread pulsar/reader_impl.go
Comment on lines +155 to +170
func (r *reader) messageID(msgID MessageID) (*messageID, bool) {
mid, ok := msgID.(*messageID)
if !ok {
r.log.Warnf("invalid message id type")
return nil, false
}

partition := mid.partitionIdx
// did we receive a valid partition index?
if partition != 0 {
r.log.Warnf("invalid partition index %d expected 0", partition)
return nil, false
}

return mid, true
}

@nitishv nitishv Jul 6, 2020

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.

Given the method signature, the purpose of this method is to convert MessageID to get *messageID
In doing so, a failure in type assertion is not really a failure. The given msgID can still be converted. Refer to a this fix I just made #305
I think that the mentioned PR change can be re-factored into this unexported method for reader, as it being used in couple of places now with this feature.

Also checking for non-zero partition index does not seem to be the purpose of this method, and should not be included here. That check is only relevant to Seek APIs, and should be moved there.

Comment thread pulsar/reader_test.go
assert.Equal(t, 10, i)
}

func TestReaderSeek(t *testing.T) {

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.

There can be an added test case which does a Seek on a custom MessageID refer to #305

@wolfstudy

Copy link
Copy Markdown
Member

ping @cornelk Can you check the @nitishv comments and fix the logic of seek for reader.

@cornelk

cornelk commented Aug 17, 2020

Copy link
Copy Markdown
Author

ping @cornelk Can you check the @nitishv comments and fix the logic of seek for reader.

pong. I am focusing on my own Pulsar Go Client now, someone else can take over this MR

@wolfstudy wolfstudy mentioned this pull request Aug 24, 2020
1 task
wolfstudy added a commit that referenced this pull request Aug 24, 2020
Signed-off-by: xiaolong.ran <rxl@apache.org>

### Motivation


Follow #222 and add the seek logic for reader 

### Modifications

- Add `seek by msgID` interface
- Add `seek by time` interface
- Add test case

### Verifying this change

- [x] Make sure that the change passes the CI checks.
@wolfstudy wolfstudy closed this Aug 24, 2020
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.

Seek function is missing in Reader

3 participants