Fix FollowingEngineTests#testProcessOnceOnPrimary for synthetic ids#143577
Fix FollowingEngineTests#testProcessOnceOnPrimary for synthetic ids#143577fcofdez merged 6 commits intoelastic:mainfrom
Conversation
This commit ensures that when synthetic id is enabled the documents have the right fields needed for them to work properly.
fcofdez
added
>test
|
Pinging @elastic/es-distributed (Team:Distributed) |
| String docId = Integer.toString(between(1, 100)); | ||
| ParsedDocument doc = randomBoolean() ? EngineTestCase.createParsedDoc(docId, null) : nestedDocFunc.apply(docId, randomInt(3)); | ||
| String docId = useSyntheticId | ||
| ? TsidExtractingIdFieldMapper.createSyntheticId(new BytesRef(Integer.toString(i)), i + 10, randomIntBetween(1, 100)) |
There was a problem hiding this comment.
I wonder if we need that synthetic id to be a valid one? Ie making sure that the generated timestamp/routing hash are correct?
There was a problem hiding this comment.
This test goes really low level and most of the validations are not in place. I think that this would suffice, but happy to harden this if you think is worth it.
There was a problem hiding this comment.
I'm mostly worried that the custom postings format could not work correctly if synthetic ids are invalid (since I saw deletes in this class).
But maybe it also doesn't use the custom codec at all?
There was a problem hiding this comment.
It uses the codec to load the synthetic id stored fields, and that seems to work. In any case, I've pushed 1affcee using a real timestamp and a non negative routing hash just in case.
tlrx
left a comment
tlrx
left a comment
There was a problem hiding this comment.
LGTM, thanks for the extra iteration.
burqen
left a comment
burqen
left a comment
There was a problem hiding this comment.
Looks good to me, just some questions / suggestions about feature flag
|
|
||
| public void testProcessOnceOnPrimary() throws Exception { | ||
| final Settings.Builder settingsBuilder = indexSettings(IndexVersion.current(), 1, 0).put("index.xpack.ccr.following_index", true); | ||
| boolean useSyntheticId = indexMode == IndexMode.TIME_SERIES && randomBoolean(); |
There was a problem hiding this comment.
Do we need to guard for feature flag here?
| boolean useSyntheticId = indexMode == IndexMode.TIME_SERIES && randomBoolean(); | |
| boolean useSyntheticId = IndexSettings.TSDB_SYNTHETIC_ID_FEATURE_FLAG && indexMode == IndexMode.TIME_SERIES && randomBoolean(); |
| case TIME_SERIES: | ||
| settingsBuilder.put("index.mode", "time_series").put("index.routing_path", "foo"); | ||
| settingsBuilder.put("index.seq_no.index_options", "points_and_doc_values"); | ||
| settingsBuilder.put(IndexSettings.SYNTHETIC_ID.getKey(), useSyntheticId); |
There was a problem hiding this comment.
Only set setting explicitly if feature flag is true (if we need to guard for that)
| settingsBuilder.put(IndexSettings.SYNTHETIC_ID.getKey(), useSyntheticId); | |
| if (IndexSettings.TSDB_SYNTHETIC_ID_FEATURE_FLAG) { | |
| settingsBuilder.put(IndexSettings.SYNTHETIC_ID.getKey(), useSyntheticId); | |
| } |
|
@elasticmachine update branch |
…lastic#143577) This commit ensures that when synthetic id is enabled; documents have the right fields needed for them to work properly.
…lastic#143577) This commit ensures that when synthetic id is enabled; documents have the right fields needed for them to work properly.
5 participants
This commit ensures that when synthetic id is enabled;
documents have the right fields needed for them to
work properly.