Skip to content

Conversation

@russcam
Copy link
Contributor

@russcam russcam commented May 3, 2019

This commit adds the SequenceNumber and PrimaryTerm properties to the Hit, MultiGetHit and GetResponse.

Closes #3708

This commit adds the SequenceNumber and PrimaryTerm properties to the Hit<T>, MultiGetHit<T> and GetResponse.

Closes #3708
@russcam
Copy link
Contributor Author

russcam commented May 3, 2019

We should consider standardising on the naming for Sequence Number, Primary Term. I've added as SequenceNumber and PrimaryTerm but notice that generated methods use SeqNoPrimaryTerm, etc.

Copy link
Contributor

@codebrain codebrain left a comment

Choose a reason for hiding this comment

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

LGTM , naming of SequenceNumber and PrimaryTerm looks good, only a minor TODO left over, but I believe it can be safely ignored.


namespace Nest
{
// TODO: Looks like this can be removed?
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related to the removal of response interfaces that @Mpdreamz was working on.

Added to discuss before GA, but not needed to address to merge this PR.

@ejsmith
Copy link
Contributor

ejsmith commented May 6, 2019

These need to be added to IBulkOperation as well.

Also, it seems a bit bad that there isn't any shared interface that contains SequenceNumber and PrimaryTerm. I currently need to check for these different types GetResponse IHit IndexResponse IMultiGetHit to get the information I need. Seems like it would be nice to have them all implement an interface.

@russcam
Copy link
Contributor Author

russcam commented May 7, 2019

@ejsmith Are there scenarios where you think having an interface for the two properties would help? I'm struggling to think of them, because aren't you always dealing with a known response or type? Do you have an example of how an interface would help, to help me see?

@ejsmith
Copy link
Contributor

ejsmith commented May 7, 2019

@russcam I have scenarios where I am doing versioning on documents for all sorts of use cases and that requires me to map those properties to and from my own models. So currently, I need to have extension methods for all of the different response and request models that have the properties instead of just a single extension method if there was a shared contract for these properties.

Also, just want to confirm that you saw that these properties are missing on IBulkOperation as I am currently blocked waiting for these.

@russcam
Copy link
Contributor Author

russcam commented May 7, 2019

@ejsmith Ok, we can introduce an interface for both properties. Suggestions for a name, ISequenceNumberPrimaryTerm?

Also, just want to confirm that you saw that these properties are missing on IBulkOperation as I am currently blocked waiting for these.

They're on bulk index and delete operations per the docs, are we missing a place?

This commit updates

- SeqNo
- SeqNoPrimaryTerm
- IfSeqNo

to expand SeqNo out to SequenceNumber, in generated types and methods, ofr consistency
@russcam
Copy link
Contributor Author

russcam commented May 7, 2019

Looking into an interface for SequenceNumber and PrimaryTerm, I don't think it'd be possible to unify types that have these properties to one interface because they may not have values on some types, depending on whether SeqNoPrimaryTerm is passed in the request. For this reason, they're modelled with long? on these types, and modelled as long in cases where they will always have values.

In order to get an alpha2 out, I'm going to merge this PR in, and we can open another issue to discuss an interface.

@russcam russcam merged commit 2156ea1 into 7.x May 7, 2019
@russcam russcam deleted the fix/3708 branch May 10, 2019 03:28
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.

4 participants