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

Patching for Issue 143 #29

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Patching for Issue 143 #29

wants to merge 3 commits into from

Conversation

lukemao
Copy link

@lukemao lukemao commented Jan 6, 2016

Based on requirements specified in KAFKA Rest API (http://docs.confluent.io/1.0.1/kafka-rest/docs/api.html#consumers):

Request JSON Array of Objects:
records – A list of records to produce to the topic.
records[i].key (object) – The message key, formatted according to the embedded format, or null to omit a key (optional)
records[i].value (object) – The message value, formatted according to the embedded format
records[i].partition (int) – Partition to store the message in (optional)

We can not provide duplicated keys when producing messages.
I updated the rest-utils to restrict number of key to be provided in message producing.

@ConfluentJenkins
Copy link
Contributor

Can one of the admins verify this patch?

@ghost
Copy link

ghost commented Jan 6, 2016

It looks like @lukemao hasn't signed our Contributor License Agreement, yet.

Appreciation of efforts,

clabot

}

public JacksonMessageBodyProvider(ObjectMapper mapper) {
mapper.enable(DeserializationFeature.FAIL_ON_READING_DUP_TREE_KEY);
Copy link
Contributor

Choose a reason for hiding this comment

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

Forcibly overriding this seems to be counter to the goal of this constructor, which is to allow the user to get control over the exact settings of the ObjectMapper. Is there any reason this needs to be overridden here? If the caller chose to leave FAIL_ON_READING_DUP_TREE_KEY off, is there any reason not to respect that?

This might require adjustments to kafka-rest to address the issue that triggered this PR, but keep in mind this class is used across a few projects and some might not need to be as restrictive and might prefer being a bit more liberal with the input they accept.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I was thinking having a single key per record to be produced is the requirement, as defined in the API reference page. Key should be uniquely identify a record. However, I think other REST function may also use this to parse their request, which may allow duplicated meta-data. I will change it back.

@lukemao
Copy link
Author

lukemao commented Jan 7, 2016

Just made another commit. Also changed the Application class to use the default constructor in JacksonMessageBodyProvider.

@ewencp
Copy link
Contributor

ewencp commented Mar 2, 2016

Can we provide a test that this actually behaves as expected? I'm not sure, but maybe it could fit into ExceptionHandlingTest since this should trigger an exception during parsing of the entity?

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.

None yet

3 participants