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

Store the reason of noop in its document tombstone #30570

Merged
merged 4 commits into from
May 15, 2018

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented May 14, 2018

Relates #29530

@dnhatn dnhatn added >feature :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. labels May 14, 2018
@dnhatn dnhatn requested review from s1monw and bleskes May 14, 2018 12:24
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

left one question

final ParsedDocument tombstone = engineConfig.getTombstoneDocSupplier().newNoopTombstoneDoc();
final BytesReference reason = BytesReference.bytes(
JsonXContent.contentBuilder().startObject().field(NoOp.REASON_FIELD_NAME, noOp.reason()).endObject());
final ParsedDocument tombstone = engineConfig.getTombstoneDocSupplier().newNoopTombstoneDoc(reason);
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we use a simple stored field? I mean do we have to store json?

@dnhatn
Copy link
Member Author

dnhatn commented May 14, 2018

@s1monw I tried to keep _source in JSON format as other docs but it was not necessary. I've updated the PR to store the reason in a raw string. Can you please have a look?

@dnhatn dnhatn requested a review from s1monw May 14, 2018 16:39
@dnhatn
Copy link
Member Author

dnhatn commented May 14, 2018

@elasticmachine test this please

final SourceToParse sourceToParse = SourceToParse.source(index, type, id, new BytesArray("{}"), XContentType.JSON);
final ParsedDocument parsedDoc = documentParser.parseDocument(sourceToParse, noopTombstoneMetadataFieldMappers).toTombstone();
// Store the reason of a noop as a raw string in the _source field
final BytesRef byteRef = new BytesRef(reason);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm doubting about this - should we always make it valid JSON : { "_reason": "bla" } ? I think it might surprising to find non json in the source field.

Copy link
Contributor

@bleskes bleskes left a comment

Choose a reason for hiding this comment

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

I raised a simple issue to discuss.

@dnhatn
Copy link
Member Author

dnhatn commented May 15, 2018

I'm doubting about this - should we always make it valid JSON : { "_reason": "bla" } ? I think it might surprising to find non json in the source field.

@bleskes Originally I made it in JSON but Simon preferred storing it as a raw string. I am fine with both approaches. @simonw WDYT?

@bleskes
Copy link
Contributor

bleskes commented May 15, 2018

I don't mind much. If Simon prefers a string, I'm good.

@dnhatn
Copy link
Member Author

dnhatn commented May 15, 2018

@elasticmachine test this please

@dnhatn
Copy link
Member Author

dnhatn commented May 15, 2018

Thanks @s1monw and @bleskes

@dnhatn dnhatn merged commit 2a2c23b into elastic:ccr May 15, 2018
@dnhatn dnhatn deleted the store-noop-reason branch May 15, 2018 17:36
dnhatn added a commit that referenced this pull request May 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. >feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants