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

Implementation of ordering key fields #892

Merged
merged 7 commits into from Jun 26, 2018

Conversation

5 participants
@rcillo
Member

rcillo commented Jun 18, 2018

https://techjira.zalando.net/browse/ARUHA-1723

Implementation of ordering key fields. The ticket refers to a more detailed discussion on why this field was requested.

I extended the original description with information I considered relevant to reduce the support burden in case users misunderstand the use case for this field.

@rcillo rcillo changed the title from POC implementation of ordering key fields to Implementation of ordering key fields Jun 25, 2018

rcillo added some commits Jun 25, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Jun 25, 2018

Codecov Report

Merging #892 into master will increase coverage by 0.02%.
The diff coverage is 75%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #892      +/-   ##
============================================
+ Coverage     53.96%   53.99%   +0.02%     
- Complexity     1756     1760       +4     
============================================
  Files           316      316              
  Lines          9680     9692      +12     
  Branches        887      888       +1     
============================================
+ Hits           5224     5233       +9     
- Misses         4132     4134       +2     
- Partials        324      325       +1
Impacted Files Coverage Δ Complexity Δ
.../java/org/zalando/nakadi/domain/EventTypeBase.java 98.75% <100%> (+0.08%) 33 <3> (+3) ⬆️
...a/org/zalando/nakadi/service/EventTypeService.java 75.09% <57.14%> (-0.5%) 54 <1> (+1)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24ff0c1...e009bb8. Read the comment docs.

@v-stepanov

This comment has been minimized.

Member

v-stepanov commented Jun 25, 2018

Looks good!

@v-stepanov

This comment has been minimized.

Member

v-stepanov commented Jun 25, 2018

👍

@rcillo

This comment has been minimized.

Member

rcillo commented Jun 25, 2018

deploy validation please

@rcillo

This comment has been minimized.

Member

rcillo commented Jun 25, 2018

👍

1 similar comment
@v-stepanov

This comment has been minimized.

Member

v-stepanov commented Jun 25, 2018

👍

It differs from `partition_key_fields` in the sense that it's not used for partitioning (known as sharding in
some systems). Also, the order of the events published in the same partition is not guaranteed to be
monotonically increased as with `partition_key_fields`. For `ordering_key_fields`, the consumer should handle

This comment has been minimized.

@ePaul

ePaul Jun 25, 2018

Member

Also, the order of the events published in the same partition is not guaranteed to be monotonically increased as with partition_key_fields.

This sentence sounds as if the partition_key_fields indicates ordering (which it doesn't, as far as I know).
Maybe instead add this to the previous paragraph:

The order indicated by ordering_key_fields can also differ from the order the events are in each partition, in case of out-of-order submission.

Another difference would be that this property applies to the whole event, not just the part inside of data, so it is possible to refer to metadata fields.

This comment has been minimized.

@rcillo

rcillo Jun 26, 2018

Member

@ePaul thanks, changed here 1804d4d

Regarding the possibility to access metadata fields, it's better described in the array item description.

type: string
description: |
Indicates a single ordering field. This is a dot separated string, which is applied
onto the whole event object, including the contained metadata and data (in

This comment has been minimized.

@ePaul

ePaul Jun 25, 2018

Member

In the API guideline proposal I wrote "JSON pointer", because that is a thing which is defined and where it is clear how it would be applied to a JSON object – for "dot separated string" this is less the case.

As I understand (from looking at the code), for partition_key_fields you are replacing the . by / and then using it as a JSON pointer – is this meant to be an implementation detail? (A JSON pointer allows some more things, like accessing an array, too.)

Differences might happen if there are field names containing a / or ..

This comment has been minimized.

@rcillo

rcillo Jun 26, 2018

Member

@ePaul the decision to use dot notation is more than 2 years and a half old. It would be hard to tell who was involved and the reason for doing so. What I can tell that in all this time, it has served users well. I don't remember users having any hard time to use it, since it's simple. You just need to describe the path to the value using dots. Based on this experience and having in mind that consistency is important, I made the choice to change it.

rcillo and others added some commits Jun 26, 2018

@rcillo

This comment has been minimized.

Member

rcillo commented Jun 26, 2018

👍

1 similar comment
@antban

This comment has been minimized.

Member

antban commented Jun 26, 2018

👍

@antban antban merged commit ca53711 into master Jun 26, 2018

6 of 7 checks passed

zappr Approval validation in progress.
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch 75% of diff hit (target 53.96%)
Details
codecov/project 53.99% (+0.02%) compared to 24ff0c1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
zappr/pr/specification PR has passed specification checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment