Skip to content

Conversation

@kkrik-es
Copy link
Contributor

Requires some refactoring due to the fact that, with routing on sort fields, ids are auto-generated. Since some tests compare base and contender results using doc ids, the auto-generated id from contender needs to be used when indexing docs on baseline side.

Related to #109334

@kkrik-es kkrik-es added >test Issues or PRs that are addressing/adding tests auto-backport Automatically create backport pull requests when merged Team:StorageEngine :StorageEngine/Logs You know, for Logs v8.18.0 labels Jan 22, 2025
@kkrik-es kkrik-es self-assigned this Jan 22, 2025
var responseBody = entityAsMap(response);
assertThat("errors in baseline bulk response:\n " + responseBody, responseBody.get("errors"), equalTo(false));
return responseBody;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All this logic feels out of place here, specific to tests using bulk indexing.. It probably belongs more to a sibling of ReindexChallengeRestIT, like BulkChallengeRestIT. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went ahead with this refactoring, ptal.

@kkrik-es kkrik-es marked this pull request as ready for review January 22, 2025 11:34
@kkrik-es kkrik-es requested a review from lkts January 22, 2025 11:34
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

@lkts
Copy link
Contributor

lkts commented Jan 22, 2025

I'll wait for this to land before merging #119994, i think it would be a much easier conflict resolve for me.

* mapping and documents in order to cover more code paths and permutations.
*/
public class StandardVersusLogsIndexModeRandomDataChallengeRestIT extends StandardVersusLogsIndexModeChallengeRestIT {
public abstract class StandardVersusLogsIndexModeRandomDataChallengeRestIT extends StandardVersusLogsIndexModeChallengeRestIT {
Copy link
Contributor

@lkts lkts Jan 22, 2025

Choose a reason for hiding this comment

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

Doesn't this effectively remove all tests that were in this class (inherited from StandardVersusLogsIndexModeChallengeRestIT)? Those are the actual logsdb vs standard random data tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These now run in BulkChallengeRestIT that inherits from StandardVersusLogsIndexModeRandomDataChallengeRestIT.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah thanks

@lkts
Copy link
Contributor

lkts commented Jan 22, 2025

I have to admit i am lost in the class hierarchy here (and i majorly contributed to that) which i am not a fan of. I don't have any specific suggestion though so LGTM modulo the question above.

@kkrik-es
Copy link
Contributor Author

I have to admit i am lost in the class hierarchy here (and i majorly contributed to that) which i am not a fan of. I don't have any specific suggestion though so LGTM modulo the question above.

It definitely feels it's deeper that it should, for a testing suite.. Here is the class hierarchy:

Screenshot 2025-01-23 at 08 29 08

I think it's good that we have symmetry between Bulk* and Reindex* now, we can probably collapse it more.

@lkts
Copy link
Contributor

lkts commented Jan 23, 2025

Can we collapse RandomDataChallenge and Challenge? Is there value in the non-random test at this point?

@kkrik-es
Copy link
Contributor Author

Can we collapse RandomDataChallenge and Challenge? Is there value in the non-random test at this point?

Not really, lemme do it and submit.

@kkrik-es
Copy link
Contributor Author

Alright moved tests with static mappings to be a leaf:

Screenshot 2025-01-23 at 19 30 46

@kkrik-es kkrik-es enabled auto-merge (squash) January 23, 2025 17:53
@kkrik-es kkrik-es disabled auto-merge January 27, 2025 09:04
# Conflicts:
#	x-pack/plugin/logsdb/src/javaRestTest/java/org/elasticsearch/xpack/logsdb/qa/StandardVersusLogsIndexModeChallengeRestIT.java
@kkrik-es kkrik-es added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jan 27, 2025
@kkrik-es kkrik-es removed the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jan 27, 2025
@kkrik-es kkrik-es enabled auto-merge (squash) January 27, 2025 15:21
@kkrik-es kkrik-es merged commit 695bf75 into elastic:main Jan 27, 2025
15 of 16 checks passed
kkrik-es added a commit to kkrik-es/elasticsearch that referenced this pull request Jan 27, 2025
…c#120584)

* Cover custom sorting and routing in randomized testing

* [CI] Auto commit changes from spotless

* fix reindex tests

* fix reindex tests

* refactor classes

* comment

* more refactoring

* more refactoring

* restore tests with static mappings

* reduce diff

* reduce diff

* Restore single-element array removal in synthetic source

* Revert "Restore single-element array removal in synthetic source"

This reverts commit e8e99e1.

* [CI] Auto commit changes from spotless

---------

Co-authored-by: elasticsearchmachine <[email protected]>
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

elasticsearchmachine pushed a commit that referenced this pull request Jan 27, 2025
… (#120932)

* Cover custom sorting and routing in randomized testing

* [CI] Auto commit changes from spotless

* fix reindex tests

* fix reindex tests

* refactor classes

* comment

* more refactoring

* more refactoring

* restore tests with static mappings

* reduce diff

* reduce diff

* Restore single-element array removal in synthetic source

* Revert "Restore single-element array removal in synthetic source"

This reverts commit e8e99e1.

* [CI] Auto commit changes from spotless

---------

Co-authored-by: elasticsearchmachine <[email protected]>
@kkrik-es kkrik-es deleted the logsdb/random-test-routing branch January 29, 2025 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :StorageEngine/Logs You know, for Logs Team:StorageEngine >test Issues or PRs that are addressing/adding tests v8.18.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants