[GitHub] bruth commented on issue #1764: WIP - Pulsar Go client library
bruth commented on issue #1764: WIP - Pulsar Go client library
URL: https://github.com/apache/incubator-pulsar/pull/1764#issuecomment-392879817
@merlimat Just ran into this using the reader example.
```
panic: interface conversion: interface {} is nil, not *pulsar.readerCallback
goroutine 17 [running, locked to thread]:
github.com/apache/incubator-pulsar/pulsar-client-go/pulsar.pulsarReaderListenerProxy(0x704158f8,
0x4c00b30, 0x482a630)
/Users/ruthb/Code/go/src/github.com/apache/incubator-pulsar/pulsar-client-go/pulsar/c_reader.go:124
+0xd7
github.com/apache/incubator-pulsar/pulsar-client-go/pulsar._cgoexpwrap_07f19b7e46c2_pulsarReaderListenerProxy(0x704158f8,
0x4c00b30, 0x482a630)
_cgo_gotypes.go:1579 +0x3f
```
It looks like the code assumes a `*readerCallback` will be set and this does
not use the assertion check:
```go
rc, ok := restorePointer(ctx).(*readerCallback)
if ok {
rc.channel <- ReaderMessage{rc.reader, newMessageWrapper(message)}
}
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] bruth commented on issue #1764: WIP - Pulsar Go client library
bruth commented on issue #1764: WIP - Pulsar Go client library URL: https://github.com/apache/incubator-pulsar/pull/1764#issuecomment-391183835 Great changes. > This I think is more related to cgo and MacOS. I have the same but it was working on linux. Good to know. I will be deploying on Linux so that works for me. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] bruth commented on issue #1764: WIP - Pulsar Go client library
bruth commented on issue #1764: WIP - Pulsar Go client library URL: https://github.com/apache/incubator-pulsar/pull/1764#issuecomment-390269446 A couple more observations: - There should be control over logging, either turning it off or provide an interface to wrap it somehow so the structure can be controlled. - I noticed I could not create a binary that statically linked libpulsar. This could be my environment, but this was the output: ``` /usr/local/Cellar/go/1.10.1/libexec/pkg/tool/darwin_amd64/link: running clang failed: exit status 1 ld: library not found for -lcrt0.o ``` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] bruth commented on issue #1764: WIP - Pulsar Go client library
bruth commented on issue #1764: WIP - Pulsar Go client library URL: https://github.com/apache/incubator-pulsar/pull/1764#issuecomment-390265202 I have a couple more comments: - Rename `MessageBuilder` to a `ProducerMessage` since it is no longer implements a builder pattern? - It may be useful to include the message in `SendAsync` error callback in order to correlate the error to the message. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] bruth commented on issue #1764: WIP - Pulsar Go client library
bruth commented on issue #1764: WIP - Pulsar Go client library URL: https://github.com/apache/incubator-pulsar/pull/1764#issuecomment-389338759 @merlimat Sounds good. I will do another pass tomorrow morning, but the code looks good to me. Maybe you can try to convince a few others in the channel to review as well. I would like to try it out locally. What build options must be used to do the build? Include `pulsar-client-cpp/lib/c` in the `CGO_LDFLAGS`? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] bruth commented on issue #1764: WIP - Pulsar Go client library
bruth commented on issue #1764: WIP - Pulsar Go client library URL: https://github.com/apache/incubator-pulsar/pull/1764#issuecomment-389285771 @merlimat Interesting, thanks for the explanation. > we just rely on best-effort acks So in other words, the command is sent to the server *optimistically* and if it fails the observation of this failure is a redelivery. Presumably (and as you said) if there is a connection issue, then the client library will retry sending the command transparently. But once it is sent we are assuming the ack will be handled. So yes in this case you are right that the `context.Context` won't actually influence the outcome. That said, I am still torn if whether it should be part of the signature anyway for consistency or if there is any chance in the future acks could be synchronous. I will leave this for you to decide. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] bruth commented on issue #1764: WIP - Pulsar Go client library
bruth commented on issue #1764: WIP - Pulsar Go client library URL: https://github.com/apache/incubator-pulsar/pull/1764#issuecomment-389009412 @merlimat Looks good. I noticed that the `Ack` calls don't take a context, but they (in theory) should. Since they interface with C, I realize that doesn't translate. But do those calls take a timeout or deadline by chance? i.e. wait N milliseconds for the ack. If so, then a timeout can be derived from the context. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] bruth commented on issue #1764: WIP - Pulsar Go client library
bruth commented on issue #1764: WIP - Pulsar Go client library URL: https://github.com/apache/incubator-pulsar/pull/1764#issuecomment-388547203 @merlimat Looking much better! I think going with the struct-based options is a good strategy. I left more comments, further cleaning up some things for consistency, etc. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
