Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support KeyValue Schema Use Null Key And Null Value #7139

Merged
merged 2 commits into from
Jun 4, 2020
Merged

Support KeyValue Schema Use Null Key And Null Value #7139

merged 2 commits into from
Jun 4, 2020

Conversation

gaoran10
Copy link
Contributor

@gaoran10 gaoran10 commented Jun 2, 2020 •
edited
Loading

Fixes #4804

Thanks, @nlu90, work at #6384.

Motivation

Currently, the KeyValue schema doesn't handle null key and null value well.

Modifications

Make the KeyValue schema support null key and null value

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Add unit test.

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

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (yes)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

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

@codelipenghui codelipenghui added this to the 2.6.0 milestone Jun 2, 2020
@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

codelipenghui
codelipenghui reviewed Jun 4, 2020
View reviewed changes
pulsar-common/src/main/proto/PulsarApi.proto Outdated
@@ -143,6 +143,9 @@ message MessageMetadata {
optional int32 num_chunks_from_msg = 27;
optional int32 total_chunk_msg_size = 28;
optional int32 chunk_id = 29;

// Indicate if the message key is set
optional bool null_key = 30 [default = false];
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to use null_partition_key here since we have partition_key and ordering key in the proto. Use null_partition_key will be more accurate.

pulsar-common/src/main/proto/PulsarApi.proto Outdated
@@ -161,6 +164,8 @@ message SingleMessageMetadata {
optional uint64 sequence_id = 8;
// Indicate if the message payload value is set
optional bool null_value = 9 [ default = false ];
// Indicate if the message key is set
optional bool null_key = 10 [ default = false];
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as the above comment.

@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

@jiazhai
Copy link
Member

jiazhai commented Jun 4, 2020

/pulsarbot run-failure-checks

@codelipenghui codelipenghui merged commit 1d5ff26 into apache:master Jun 4, 2020
@codelipenghui codelipenghui mentioned this pull request Jun 4, 2020
Closed
1 task
zeo1995 pushed a commit to zeo1995/pulsar that referenced this pull request Jun 5, 2020
Merge branch 'website-update' of github.com:zeo1995/pulsar into websi…
128b8d5 
…te-update

* 'website-update' of github.com:zeo1995/pulsar: (432 commits)
  Fixed ordering issue in KeyShared dispatcher when adding consumer (apache#7106)
  Fix Duplicated messages are sent to dead letter topic apache#6960 (apache#7021)
  [Issue 2793][Doc]--Update the TLS hostname verification for CPP and Python clients (apache#7162)
  [Doc]--set netty mex frame size (apache#7174)
  [Doc] Update for the maximum message size (apache#7171)
  Fixed KeyShared consumers getting stuck on delivery (apache#7105)
  [apache#6003][pulsar-functions] Possibility to add builtin Functions (apache#6895)
  [Issue 6921][pulsar-broker-common] Replaced "Paths.get(...).getParent()", because it's system dependent and uses '\' as path separator on Windows (apache#6992)
  Improve broker unit test CI (apache#7173)
  Fix typo in exception message (apache#7027)
  Support KeyValue Schema Use Null Key And Null Value (apache#7139)
  [Doc]--Update documents for support consumer priority level in failover mode (apache#7136)
  Add schema config to cpp and cgo docs. (apache#7137)
  [Doc]--Update for the maximum message size (apache#7160)
  [C++] Expose ZSTD and Snappy compression to C API (apache#7014)
  [pulsar-proxy] add proxyLogLevel into config file (apache#6948)
  Add multi-hosts example for bookkeeperMetadataServiceUri (apache#6998)
  support for termination of partitioned topic (apache#6126)
  Use pure-java Air-Compressor instead of JNI based libraries (apache#5390)
  [Issues 5709]remove the namespace checking (apache#5716)
  ...

# Conflicts:
#	site2/website/scripts/split-swagger-by-version.js
@sijie sijie mentioned this pull request Jun 10, 2020
Closed
@fmiguelez fmiguelez mentioned this pull request Jun 29, 2020
Closed
@fmiguelez
Copy link
Contributor

This does not work as described here

@fmiguelez fmiguelez mentioned this pull request Jul 1, 2020
Open
This was referenced Jul 1, 2020
Closed
Open
cdbartholomew pushed a commit to kafkaesque-io/pulsar that referenced this pull request Jul 24, 2020
@gaoran10 @cdbartholomew
Support KeyValue Schema Use Null Key And Null Value (apache#7139)
6bc8af2 
Fixes apache#4804  

Thanks, @nlu90, work at apache#6384.

### Motivation

Currently, the KeyValue schema doesn't handle `null` key and `null` value well.
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
@gaoran10 @huangdx0726
Support KeyValue Schema Use Null Key And Null Value (apache#7139)
6d08a44 
Fixes apache#4804  

Thanks, @nlu90, work at apache#6384.

### Motivation

Currently, the KeyValue schema doesn't handle `null` key and `null` value well.
@gaoran10 gaoran10 deleted the support-keyvalue-null branch September 24, 2020 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codelipenghui codelipenghui codelipenghui approved these changes

@jiazhai jiazhai jiazhai approved these changes

@sijie sijie Awaiting requested review from sijie

@rdhabalia rdhabalia Awaiting requested review from rdhabalia

@merlimat merlimat Awaiting requested review from merlimat

Assignees

@gaoran10 gaoran10

Labels
None yet
Projects
None yet
Milestone
2.6.0
Development

Successfully merging this pull request may close these issues.

KeyValue schema doesn't handle null key and null value well
4 participants
@gaoran10 @codelipenghui @jiazhai @fmiguelez