Skip to content

Conversation

alexey-ivanov-es
Copy link
Contributor

@alexey-ivanov-es alexey-ivanov-es commented Dec 19, 2024

Restore contentTypeVersion check in RestCompatibleVersionHelper which was commented out when RestApiVersion.V_9 was introduced, update tests to work correctly with v9 version and remove @UpdateForV9 annotation that is no longer applicable

public static final String THREAD_POOL_METRIC_NAME_REJECTED = ".threads.rejected.total";

public enum ThreadPoolType {
@Deprecated(forRemoval = true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR for 8.x: #118492

@alexey-ivanov-es alexey-ivanov-es marked this pull request as ready for review December 19, 2024 17:29
@alexey-ivanov-es alexey-ivanov-es requested a review from a team as a code owner December 19, 2024 17:29
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Dec 19, 2024
super(NAME);
}

@UpdateForV9(owner = UpdateForV9.Owner.CORE_INFRA) // this can be replaced with TransportRequest.Empty in v9
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Slack thread confirming that we don't need it anymore: https://elastic.slack.com/archives/C8UUBNASY/p1733330314431679

@alexey-ivanov-es alexey-ivanov-es added :Core/Infra/Core Core issues without another label >refactoring and removed needs:triage Requires assignment of a team area label labels Dec 19, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Dec 19, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

The changes look fine, but they still seem unrelated and the PR title is not descriptive of the actual change. Can you change the title to a meaningful description, or further split out these changes so that the git log clearly shows what changed?

public static String TEST_SYSTEM_INDEX_PATTERN = ".test*";
private static final IndexVersion TEST_OLD_VERSION = IndexVersion.fromId(6000099);
// Version just before MINIMUM_COMPATIBLE in order to check that UpgradeStatus.MIGRATION_NEEDED is set correctly
private static final IndexVersion TEST_OLD_VERSION = IndexVersion.fromId(IndexVersions.MINIMUM_COMPATIBLE.id() - 100);
Copy link
Member

Choose a reason for hiding this comment

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

Why 100? Although in the past IndexVersions were stepped by 100 because they were just Version, it seems like this test would be more future proof if we subtract just 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it did this because IndexVersions stepped by 100, changed to 1

@alexey-ivanov-es
Copy link
Contributor Author

@rjernst I've extracted the changes related to ThreadPools into a separate PR ([#119941]). The remaining changes, while indeed unrelated, are quite small - mostly uncommenting existing code or minor test updates. I don't think it would be worth splitting them further into multiple PRs containing 1-2 line adjustments without any significant impact on product behavior. I've improved the PR description to clarify the changes better

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM. Please also update the PR title since that will be part of the commit message.

@alexey-ivanov-es alexey-ivanov-es merged commit 873dc52 into elastic:main Jan 13, 2025
16 checks passed
@alexey-ivanov-es alexey-ivanov-es deleted the ES-9378_misc branch January 13, 2025 12:03
martijnvg pushed a commit to martijnvg/elasticsearch that referenced this pull request Jan 14, 2025
…stic#119105)

Restore contentTypeVersion check in RestCompatibleVersionHelper which was commented out when RestApiVersion.V_9 was introduced, update tests to work correctly with v9 version and remove @UpdateForV9 annotation that is no longer applicable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >refactoring Team:Core/Infra Meta label for core/infra team v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants