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.
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
Add metadata to StreamConfig and ConsumerConfig #3797
Conversation
@wallyqs PR updated to include limits we agreed upon in ADR. |
604f736
to
81c759d
Compare
metadataLen += len(k) + len(v) | ||
} | ||
if metadataLen > JSMaxMetadataLen { | ||
return NewJSConsumerMetadataLengthError(fmt.Sprintf("%dKB", JSMaxMetadataLen/1024)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check for empty keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to be as transparent as possible on the server side and add sanity checks on the client side (according to client).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty keys are persisted but seems benign to support that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I think we can leave it as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
server/jetstream_errors_generated.go
Outdated
@@ -446,6 +449,7 @@ var ( | |||
JSConsumerMaxRequestBatchNegativeErr: {Code: 400, ErrCode: 10114, Description: "consumer max request batch needs to be > 0"}, | |||
JSConsumerMaxRequestExpiresToSmall: {Code: 400, ErrCode: 10115, Description: "consumer max request expires needs to be >= 1ms"}, | |||
JSConsumerMaxWaitingNegativeErr: {Code: 400, ErrCode: 10087, Description: "consumer max waiting needs to be positive"}, | |||
JSConsumerMetadataLengthErrF: {Code: 400, ErrCode: 10134, Description: "Consumer metadata exceeds maximum size of {limit}"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSConsumerMetadataLengthErrF: {Code: 400, ErrCode: 10134, Description: "Consumer metadata exceeds maximum size of {limit}"}, | |
JSConsumerMetadataLengthErrF: {Code: 400, ErrCode: 10134, Description: "consumer metadata exceeds maximum size of {limit}"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
casing issue here, the error below has the same issue though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, fixed.
@@ -446,6 +449,7 @@ var ( | |||
JSConsumerMaxRequestBatchNegativeErr: {Code: 400, ErrCode: 10114, Description: "consumer max request batch needs to be > 0"}, | |||
JSConsumerMaxRequestExpiresToSmall: {Code: 400, ErrCode: 10115, Description: "consumer max request expires needs to be >= 1ms"}, | |||
JSConsumerMaxWaitingNegativeErr: {Code: 400, ErrCode: 10087, Description: "consumer max waiting needs to be positive"}, | |||
JSConsumerMetadataLengthErrF: {Code: 400, ErrCode: 10134, Description: "Consumer metadata exceeds maximum size of {limit}"}, | |||
JSConsumerNameContainsPathSeparatorsErr: {Code: 400, ErrCode: 10127, Description: "Consumer name can not contain path separators"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSConsumerNameContainsPathSeparatorsErr: {Code: 400, ErrCode: 10127, Description: "Consumer name can not contain path separators"}, | |
JSConsumerNameContainsPathSeparatorsErr: {Code: 400, ErrCode: 10127, Description: "consumer name can not contain path separators"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
server/consumer.go
Outdated
@@ -94,6 +94,9 @@ type ConsumerConfig struct { | |||
|
|||
// Don't add to general clients. | |||
Direct bool `json:"direct,omitempty"` | |||
|
|||
// Additional metadata for the Consumer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Additional metadata for the Consumer. | |
// Metadata is additional metadata for the Consumer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
server/stream.go
Outdated
@@ -80,6 +80,9 @@ type StreamConfig struct { | |||
// AllowRollup allows messages to be placed into the system and purge | |||
// all older messages using a special msg header. | |||
AllowRollup bool `json:"allow_rollup_hdrs"` | |||
|
|||
// Additional metadata for the Stream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Additional metadata for the Stream. | |
// Metadata is additional metadata for the Stream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, good point. Fixed both.
We should do a sweep through comments and enable lint for that.
ee0b82e
to
6d482a8
Compare
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
6d482a8
to
9ab52d4
Compare
Adds
Metadata
field toConsumerConfig
andStreamConfig
, allowing for a convenient way of adding additional information to those, better than usingDescription
.When merged, will follow up with ADR for clients.
Signed-off-by: Tomasz Pietrek tomasz@nats.io
/cc @nats-io/core