You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/05/15 05:25:15 UTC

[GitHub] [pulsar-client-go] cornelk opened a new pull request #222: WIP: [Issue 218] Add Seek functions to Reader

cornelk opened a new pull request #222:
URL: https://github.com/apache/pulsar-client-go/pull/222


   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)
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-go] cornelk commented on pull request #222: WIP: [Issue 218] Add Seek functions to Reader

Posted by GitBox <gi...@apache.org>.
cornelk commented on pull request #222:
URL: https://github.com/apache/pulsar-client-go/pull/222#issuecomment-674720519


   > 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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-go] wolfstudy commented on pull request #222: WIP: [Issue 218] Add Seek functions to Reader

Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #222:
URL: https://github.com/apache/pulsar-client-go/pull/222#issuecomment-674633994


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-go] nitishv commented on a change in pull request #222: WIP: [Issue 218] Add Seek functions to Reader

Posted by GitBox <gi...@apache.org>.
nitishv commented on a change in pull request #222:
URL: https://github.com/apache/pulsar-client-go/pull/222#discussion_r449955128



##########
File path: pulsar/reader_impl.go
##########
@@ -150,3 +151,33 @@ func (r *reader) hasMoreMessages() bool {
 func (r *reader) Close() {
 	r.pc.Close()
 }
+
+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
+}

Review comment:
       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 https://github.com/apache/pulsar-client-go/pull/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.
   
   
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-go] wolfstudy commented on pull request #222: WIP: [Issue 218] Add Seek functions to Reader

Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #222:
URL: https://github.com/apache/pulsar-client-go/pull/222#issuecomment-629034048


   ```
   panic: test timed out after 20m0s
   ```


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-go] nitishv commented on a change in pull request #222: WIP: [Issue 218] Add Seek functions to Reader

Posted by GitBox <gi...@apache.org>.
nitishv commented on a change in pull request #222:
URL: https://github.com/apache/pulsar-client-go/pull/222#discussion_r449955387



##########
File path: pulsar/reader_test.go
##########
@@ -362,3 +365,108 @@ func TestReaderHasNext(t *testing.T) {
 
 	assert.Equal(t, 10, i)
 }
+
+func TestReaderSeek(t *testing.T) {

Review comment:
       There can be an added test case which does a Seek on a custom MessageID refer to https://github.com/apache/pulsar-client-go/pull/305

##########
File path: pulsar/reader_impl.go
##########
@@ -150,3 +151,33 @@ func (r *reader) hasMoreMessages() bool {
 func (r *reader) Close() {
 	r.pc.Close()
 }
+
+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
+}

Review comment:
       Given the method name, the purpose of this method is to convert MessageID to get *messageID
   In doing so failure in type assertion is not really a failure. The given msgID can still be converted. Refer to a this fix I just made https://github.com/apache/pulsar-client-go/pull/305
   
   I think that change can be re-factored into this unexported method for reader, as it being used in couple of places now with this change.
   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.
   
   
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-go] wolfstudy commented on pull request #222: WIP: [Issue 218] Add Seek functions to Reader

Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #222:
URL: https://github.com/apache/pulsar-client-go/pull/222#issuecomment-640972693


   Move this change to 0.2.0


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-go] wolfstudy closed pull request #222: WIP: [Issue 218] Add Seek functions to Reader

Posted by GitBox <gi...@apache.org>.
wolfstudy closed pull request #222:
URL: https://github.com/apache/pulsar-client-go/pull/222


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-go] wolfstudy closed pull request #222: WIP: [Issue 218] Add Seek functions to Reader

Posted by GitBox <gi...@apache.org>.
wolfstudy closed pull request #222:
URL: https://github.com/apache/pulsar-client-go/pull/222


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org