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.

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

Update producer and consumer to allow sending and receiving Key/Value records #828

Merged
merged 1 commit into from
Mar 8, 2021

Conversation

nicholastmosher
Copy link
Contributor

@nicholastmosher nicholastmosher commented Mar 2, 2021

Here's a list of changes in this PR:

  • Added Record::key(&self) -> &[u8] and Record::value(&self) -> &[u8] in the consumer
  • Removed impl AsRef<[u8]> for Record in consumer because we now want to explicitly distinguish between keys and values
  • Added TopicProducer::send_key_value_record(&self, key: K, value: V, partition: i32) to producer
  • Changed the representation of record keys in the protocol from key: B to key: Option<B> (where B is a byte buffer)
  • Added a test that shows that decoding a record with an old encoding just so happens to work as expected
    • Old records are always represented as "empty keys" in the form of zero-length buffers
    • Zero-length buffers are encoded as a length whose value is zero 0x00.
    • The new record encoding represents "absent keys" in the form of Option::None
    • Option::None tag is encoded as 0x00 (as opposed to Option::Some, whose tag is 0x01)
    • Therefore, decoding old records with empty keys into new records with absent keys works out-of-the-box
    • However, we must never attempt to decode a new record with any key using the older decoder.

Some potentially-open-questions:

  • Are we happy with the new function names? These names are all potentially up for debate:
    • Record::key
    • Record::value
    • TopicProducer::send_key_value_record
    • TopicProducer::send

@ajhunyady
Copy link
Contributor

@nicholastmosher, is there a sample CLI on how to use this feature? We need to prepare a simple app or blog to introduce it.

@ajhunyady
Copy link
Contributor

Hmmm... this looks very complicated to me and it shouldn't be.

@sehz
Copy link
Contributor

sehz commented Mar 2, 2021

For CLI, here is one way to do:

$ fluvio produce --key-seperator="="
name=John Doe 
country=US

@sehz
Copy link
Contributor

sehz commented Mar 2, 2021

We should simplify API much as possible.
In most cases, K/V will be typical:

producer.send("name","doe")

For case, where only values is send:

producer.send(None,"doe")

@ajhunyady
Copy link
Contributor

I suggest keeping the .send() for values and have an alternative API for send KV. That way the most common use case send will remain unchanged.

@sehz
Copy link
Contributor

sehz commented Mar 2, 2021

K/V will be more typical use case rather than other way around.

@ajhunyady
Copy link
Contributor

In that case, send it is...

@ajhunyady
Copy link
Contributor

ajhunyady commented Mar 2, 2021

For CLI, here is one way to do:

$ fluvio produce --key-seperator="="
name=John Doe 
country=US

There should be some indication this is a key/value record, I assume is `--key-separator
I would use an example that's more realistic

$ fluvio produce --key-separator="="
cust_100="John Doe, Los Angeles, CA, USA" 

@sehz
Copy link
Contributor

sehz commented Mar 2, 2021

--kv to indicate kv value.

$ fluvio produce --kv --separator "="
name=John Doe 
country=US

examples/00-produce/src/main.rs Outdated Show resolved Hide resolved
src/client/src/consumer.rs Show resolved Hide resolved
@nicholastmosher nicholastmosher linked an issue Mar 4, 2021 that may be closed by this pull request
4 tasks
@nicholastmosher nicholastmosher force-pushed the key-value-option branch 2 times, most recently from 846833a to 1df9a3a Compare March 5, 2021 14:15
@sehz sehz requested a review from simlay March 5, 2021 20:02
examples/01-produce-key-value/src/main.rs Show resolved Hide resolved
examples/02-consume/src/main.rs Outdated Show resolved Hide resolved
src/client/src/producer.rs Show resolved Hide resolved
src/dataplane-protocol/src/record.rs Outdated Show resolved Hide resolved
src/dataplane-protocol/src/record.rs Outdated Show resolved Hide resolved
src/dataplane-protocol/src/record.rs Outdated Show resolved Hide resolved
src/dataplane-protocol/src/record.rs Outdated Show resolved Hide resolved
src/dataplane-protocol/tests/file_fetch.rs Outdated Show resolved Hide resolved
src/dataplane-protocol/tests/file_fetch.rs Outdated Show resolved Hide resolved
@sehz
Copy link
Contributor

sehz commented Mar 5, 2021

@simlay Can you review this as well?

src/client/src/consumer.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@simlay simlay left a comment

Choose a reason for hiding this comment

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

Sehyo managed to cover most of my concerns about things. I had some minor nits.

Also what's the file src/dataplane-protocol/tests/test_old_record_empty_key.bin?

src/client/src/producer.rs Show resolved Hide resolved
src/client/src/lib.rs Show resolved Hide resolved
@nicholastmosher
Copy link
Contributor Author

@simlay that .bin file is what I used to verify that records in the new schema can be successfully decoded from old records that were encoded with the old schema. See https://github.com/infinyon/fluvio/pull/828/files#diff-e50536c96490b842b8f91fab3fca42353b0beca28ebe99ecd1eb80bcfc53c4b6R568-R606

Basically, I used the old encoder to encode a specific record into bytes, and saved those bytes into test_old_record_empty_key.bin. Then I wrote a test which attempts to decode a new-schema-record from those same bytes, and assert that the data landed where it was supposed to in the new record.

Copy link
Contributor

@simlay simlay left a comment

Choose a reason for hiding this comment

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

I think I'm good with this.

src/client/src/lib.rs Show resolved Hide resolved
src/client/src/producer.rs Show resolved Hide resolved
@sehz
Copy link
Contributor

sehz commented Mar 8, 2021

Can you squash commits and produce clean commit logs?

Remove value-only constructors for Record

Fix doc tests and remove unused imports

Save K/V without Option in protocol

Use Option for key in protocol

Write test for decoding old record as new record

Apply cargo clippy

Rename send_key_value_records to send

Bump fluvio client version to 0.6.0

Create example for key-value producer

Add key-value example to Fluvio client docs

Fix example tests

Bump up consumer test-runner timeout

Review: Add constructors for DefaultRecord

Add back From<String> for Record<B>

Review: Fix extra ref in example

Add back AsRef<[u8]> for Record

Add key() and value() methods to DefaultRecord

Add back value-only constructors for dataplane Record

Add into_inner() for consumer Record
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking: Support Key/value in produce and consumer
4 participants