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/27 03:28:15 UTC
[GitHub] [pulsar-client-go] keithnull opened a new pull request #263: [Issue 240] Add check for max message size
keithnull opened a new pull request #263:
URL: https://github.com/apache/pulsar-client-go/pull/263
Fixes #240
### Motivation
Issue #240: Add client-side check for max message size
### Modifications
1. When creating a connection, try to get maxMessageSize from handshake
response command. If it's not set, then use the default maxMessageSize
value defined in the client side.
2. When sending a message, check whether the size of payload exceeds
maxMessageSize. If so, return error immediately without adding this
meesage into sending queue.
3. To implement these, I made some tiny modifications in Connection
interface and added a field in its implementation struct.
----------------------------------------------------------------
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 a change in pull request #263: [Issue 240] Add check for max message size
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on a change in pull request #263:
URL: https://github.com/apache/pulsar-client-go/pull/263#discussion_r431529838
##########
File path: pulsar/producer_partition.go
##########
@@ -236,6 +239,16 @@ func (p *partitionProducer) internalSend(request *sendRequest) {
msg := request.msg
+ // if msg is too large
+ if len(msg.Payload) > int(p.cnx.GetMaxMessageSize()) {
+ p.publishSemaphore.Release()
+ request.callback(nil, request.msg, errMessageTooLarge)
+ p.log.WithField("size", len(msg.Payload)).
+ WithField("properties", msg.Properties).
+ Error("message size exceeds MaxMessageSize")
Review comment:
Thanks @jinfengnarvar thanks for pointing this out.
Currently, there are some inconsistencies in the handling of errors. In order to ensure the structured output of the log, we should use `withError()` instead of `Error()`.
I send an [issue](https://github.com/apache/pulsar-client-go/issues/264) to track this issue.
----------------------------------------------------------------
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 a change in pull request #263: [Issue 240] Add check for max message size
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on a change in pull request #263:
URL: https://github.com/apache/pulsar-client-go/pull/263#discussion_r431529838
##########
File path: pulsar/producer_partition.go
##########
@@ -236,6 +239,16 @@ func (p *partitionProducer) internalSend(request *sendRequest) {
msg := request.msg
+ // if msg is too large
+ if len(msg.Payload) > int(p.cnx.GetMaxMessageSize()) {
+ p.publishSemaphore.Release()
+ request.callback(nil, request.msg, errMessageTooLarge)
+ p.log.WithField("size", len(msg.Payload)).
+ WithField("properties", msg.Properties).
+ Error("message size exceeds MaxMessageSize")
Review comment:
Thanks @jinfengnarvar thanks for pointing this out.
As defined by WithError:
```
func WithError(err error) *Entry {
return std.WithField(ErrorKey, err)
}
```
It requires an error type signature as an input parameter, but sometimes we just want to output an error log, so for errors that are not explicitly returned, we will use `Error()` instead of `WithError()`
For this change of @keithnull, we should adopt the scheme provided by @jinfengnarvar, use `WithError()` instead of `Error()`.
I send an [issue:246](https://github.com/apache/pulsar-client-go/issues/264) to track this issue. @keithnull Can you help to check if we have similar problems in the project, if not, please close this issue.
----------------------------------------------------------------
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 a change in pull request #263: [Issue 240] Add check for max message size
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on a change in pull request #263:
URL: https://github.com/apache/pulsar-client-go/pull/263#discussion_r431529838
##########
File path: pulsar/producer_partition.go
##########
@@ -236,6 +239,16 @@ func (p *partitionProducer) internalSend(request *sendRequest) {
msg := request.msg
+ // if msg is too large
+ if len(msg.Payload) > int(p.cnx.GetMaxMessageSize()) {
+ p.publishSemaphore.Release()
+ request.callback(nil, request.msg, errMessageTooLarge)
+ p.log.WithField("size", len(msg.Payload)).
+ WithField("properties", msg.Properties).
+ Error("message size exceeds MaxMessageSize")
Review comment:
Thanks @jinfengnarvar thanks for pointing this out.
Currently, there are some inconsistencies in the handling of errors. In order to ensure the structured output of the log, we should use `withError()` instead of `Error()`.
----------------------------------------------------------------
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] keithnull commented on pull request #263: [Issue 240] Add check for max message size
Posted by GitBox <gi...@apache.org>.
keithnull commented on pull request #263:
URL: https://github.com/apache/pulsar-client-go/pull/263#issuecomment-633997298
@wolfstudy okay, I'll try to add some test cases for this change later
----------------------------------------------------------------
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] jinfengnarvar commented on a change in pull request #263: [Issue 240] Add check for max message size
Posted by GitBox <gi...@apache.org>.
jinfengnarvar commented on a change in pull request #263:
URL: https://github.com/apache/pulsar-client-go/pull/263#discussion_r430382440
##########
File path: pulsar/producer_partition.go
##########
@@ -236,6 +239,16 @@ func (p *partitionProducer) internalSend(request *sendRequest) {
msg := request.msg
+ // if msg is too large
+ if len(msg.Payload) > int(p.cnx.GetMaxMessageSize()) {
+ p.publishSemaphore.Release()
+ request.callback(nil, request.msg, errMessageTooLarge)
+ p.log.WithField("size", len(msg.Payload)).
+ WithField("properties", msg.Properties).
+ Error("message size exceeds MaxMessageSize")
Review comment:
Curious why not use `.WithError(errMessageTooLarge).Error()`?
##########
File path: pulsar/producer_partition.go
##########
@@ -236,6 +239,16 @@ func (p *partitionProducer) internalSend(request *sendRequest) {
msg := request.msg
+ // if msg is too large
+ if len(msg.Payload) > int(p.cnx.GetMaxMessageSize()) {
+ p.publishSemaphore.Release()
+ request.callback(nil, request.msg, errMessageTooLarge)
+ p.log.WithField("size", len(msg.Payload)).
+ WithField("properties", msg.Properties).
+ Error("message size exceeds MaxMessageSize")
Review comment:
I'm even newer to the project :) @wolfstudy ?
----------------------------------------------------------------
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 merged pull request #263: [Issue 240] Add check for max message size
Posted by GitBox <gi...@apache.org>.
wolfstudy merged pull request #263:
URL: https://github.com/apache/pulsar-client-go/pull/263
----------------------------------------------------------------
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] keithnull commented on a change in pull request #263: [Issue 240] Add check for max message size
Posted by GitBox <gi...@apache.org>.
keithnull commented on a change in pull request #263:
URL: https://github.com/apache/pulsar-client-go/pull/263#discussion_r430450652
##########
File path: pulsar/producer_partition.go
##########
@@ -236,6 +239,16 @@ func (p *partitionProducer) internalSend(request *sendRequest) {
msg := request.msg
+ // if msg is too large
+ if len(msg.Payload) > int(p.cnx.GetMaxMessageSize()) {
+ p.publishSemaphore.Release()
+ request.callback(nil, request.msg, errMessageTooLarge)
+ p.log.WithField("size", len(msg.Payload)).
+ WithField("properties", msg.Properties).
+ Error("message size exceeds MaxMessageSize")
Review comment:
Oh, thanks for pointing this out~
As I'm new to this project, when I was writing these lines, I mainly referred to other error handling lines in the same function, which use `.Error("xxx")` rather than `.WithError()`.
But I think your idea is better, so maybe we need to rewrite other error handling lines as well?
##########
File path: pulsar/producer_partition.go
##########
@@ -236,6 +239,16 @@ func (p *partitionProducer) internalSend(request *sendRequest) {
msg := request.msg
+ // if msg is too large
+ if len(msg.Payload) > int(p.cnx.GetMaxMessageSize()) {
+ p.publishSemaphore.Release()
+ request.callback(nil, request.msg, errMessageTooLarge)
+ p.log.WithField("size", len(msg.Payload)).
+ WithField("properties", msg.Properties).
+ Error("message size exceeds MaxMessageSize")
Review comment:
Oh, thanks for pointing this out~
As I'm new to this project, when I was writing these lines, I mainly referred to other error handling lines in the same function, which use `.Error("xxx")` rather than `.WithError()`. So I wrote it this way for consistency.
But I think your idea is better, so maybe we need to rewrite other error handling lines as well?
----------------------------------------------------------------
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 a change in pull request #263: [Issue 240] Add check for max message size
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on a change in pull request #263:
URL: https://github.com/apache/pulsar-client-go/pull/263#discussion_r431529838
##########
File path: pulsar/producer_partition.go
##########
@@ -236,6 +239,16 @@ func (p *partitionProducer) internalSend(request *sendRequest) {
msg := request.msg
+ // if msg is too large
+ if len(msg.Payload) > int(p.cnx.GetMaxMessageSize()) {
+ p.publishSemaphore.Release()
+ request.callback(nil, request.msg, errMessageTooLarge)
+ p.log.WithField("size", len(msg.Payload)).
+ WithField("properties", msg.Properties).
+ Error("message size exceeds MaxMessageSize")
Review comment:
Thanks @jinfengnarvar thanks for pointing this out.
Currently, there are some inconsistencies in the handling of errors. In order to ensure the structured output of the log, we should use `withError()` instead of `Error()`.
I send an [issue:246](https://github.com/apache/pulsar-client-go/issues/264) to track this issue.
----------------------------------------------------------------
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