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

docs: Bring KLIP-15 up to date #4499

Merged
merged 4 commits into from
Feb 14, 2020
Merged

Conversation

purplefox
Copy link
Contributor

Description

Update KLIP to bring up to date wrt current development and latest PR comments

Testing done

N/A

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@purplefox purplefox requested a review from a team as a code owner February 10, 2020 09:09
@purplefox purplefox changed the title docs: Bring klip up to date docs: Bring KLIP-15 up to date Feb 10, 2020
Copy link
Contributor

@colinhicks colinhicks left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -89,8 +93,6 @@ operation (newlines have been added here for the sake of clarity but the real JS
````
Copy link
Contributor

Choose a reason for hiding this comment

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

Not part of the review changes, but above:

(newlines have been added here for the sake of clarity but the real JSON must not contain newlines)

we might want to clarify "real JSON must not contain unescaped newlines"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

Copy link
Contributor

@vcrfxia vcrfxia left a comment

Choose a reason for hiding this comment

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

LGTM with some questions inline. Thanks!

@@ -133,14 +132,14 @@ Push queries can be explicitly terminated by the client by making a request to t

The request method will be a POST.

Requests will be made to a specific URL, e.g. "/query-terminate" (this can be configurable)
Requests will be made to a specific URL, e.g. "/query-close" (this can be configurable)
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 close-query and not query-close, right?

Also, what does "(this can be configurable)" mean?


All ack responses also contain a field `seq` with a 64 bit signed integer value. This number
corresponds to the sequence of the insert on the request. The first send has sequence `0`, the second
`1`, the third `3`, etc. It allows the client to correlate the ack to the corresponding send.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be "the third 2" instead of "the third 3", because of the 0-indexing?

In case of error, an error response (see below) will be sent. For an error response for a send, the
`seq` field will also be included.

Please note that acks can be returned in a different sequence to which the inserts were submitted.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we guarantee the order of the inserts, even if the order of the acks are not guaranteed, or is the order of the inserts not guaranteed either? Is this because of a limitation of the streaming protocol or something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm, found the answer in discussion on the original PR: #4069 (comment)

@purplefox purplefox merged commit ddf0701 into confluentinc:master Feb 14, 2020
@purplefox purplefox deleted the klip15-updates branch April 28, 2020 07:20
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.

3 participants