From 45a8ed41f0c67f8853e36d95c1cc05d921b92739 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 16 Sep 2024 11:09:17 -0400 Subject: [PATCH 1/6] Bump com.gradle.develocity from 3.18 to 3.18.1 (#15947) Bumps com.gradle.develocity from 3.18 to 3.18.1. --- updated-dependencies: - dependency-name: com.gradle.develocity dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- settings.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/settings.gradle b/settings.gradle index b79c2aee135fc..8412d198a2a29 100644 --- a/settings.gradle +++ b/settings.gradle @@ -10,7 +10,7 @@ */ plugins { - id "com.gradle.develocity" version "3.18" + id "com.gradle.develocity" version "3.18.1" } ext.disableBuildCache = hasProperty('DISABLE_BUILD_CACHE') || System.getenv().containsKey('DISABLE_BUILD_CACHE') From e3bbc74868dcba47b4231c8d6e095c2721d019e3 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 16 Sep 2024 11:40:39 -0400 Subject: [PATCH 2/6] Bump ch.qos.logback:logback-core from 1.5.6 to 1.5.8 in /test/fixtures/hdfs-fixture (#15946) * Bump ch.qos.logback:logback-core in /test/fixtures/hdfs-fixture Bumps [ch.qos.logback:logback-core](https://github.com/qos-ch/logback) from 1.5.6 to 1.5.8. - [Commits](https://github.com/qos-ch/logback/compare/v_1.5.6...v_1.5.8) --- updated-dependencies: - dependency-name: ch.qos.logback:logback-core dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] * Update changelog Signed-off-by: dependabot[bot] --------- Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] --- CHANGELOG.md | 1 + test/fixtures/hdfs-fixture/build.gradle | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec4ac7504b9c2..cc313fdcdb849 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Bump `org.apache.logging.log4j:log4j-core` from 2.23.1 to 2.24.0 ([#15858](https://github.com/opensearch-project/OpenSearch/pull/15858)) - Bump `peter-evans/create-pull-request` from 6 to 7 ([#15863](https://github.com/opensearch-project/OpenSearch/pull/15863)) - Bump `com.nimbusds:oauth2-oidc-sdk` from 11.9.1 to 11.19.1 ([#15862](https://github.com/opensearch-project/OpenSearch/pull/15862)) +- Bump `ch.qos.logback:logback-core` from 1.5.6 to 1.5.8 ([#15946](https://github.com/opensearch-project/OpenSearch/pull/15946)) ### Changed diff --git a/test/fixtures/hdfs-fixture/build.gradle b/test/fixtures/hdfs-fixture/build.gradle index b5cd12ef0c11f..20fd1058a181d 100644 --- a/test/fixtures/hdfs-fixture/build.gradle +++ b/test/fixtures/hdfs-fixture/build.gradle @@ -74,7 +74,7 @@ dependencies { api 'org.apache.zookeeper:zookeeper:3.9.2' api "org.apache.commons:commons-text:1.12.0" api "commons-net:commons-net:3.11.1" - api "ch.qos.logback:logback-core:1.5.6" + api "ch.qos.logback:logback-core:1.5.8" api "ch.qos.logback:logback-classic:1.2.13" api "org.jboss.xnio:xnio-nio:3.8.16.Final" api 'org.jline:jline:3.26.3' From 81288b1044348d99cce6d23770755a464121f7cc Mon Sep 17 00:00:00 2001 From: Himshikha Gupta Date: Wed, 18 Sep 2024 00:28:59 +0530 Subject: [PATCH 3/6] remote publication checksum stats (#15957) * Remote publication checksum stats Signed-off-by: Himshikha Gupta Co-authored-by: Himshikha Gupta --- .../remote/RemoteStatePublicationIT.java | 19 ++++++++++ .../remote/RemoteClusterStateService.java | 6 ++++ .../gateway/remote/RemoteDownloadStats.java | 36 +++++++++++++++++++ .../remote/RemotePersistenceStats.java | 24 ++++++++++--- .../RemoteClusterStateServiceTests.java | 10 ++++++ 5 files changed, 91 insertions(+), 4 deletions(-) create mode 100644 server/src/main/java/org/opensearch/gateway/remote/RemoteDownloadStats.java diff --git a/server/src/internalClusterTest/java/org/opensearch/gateway/remote/RemoteStatePublicationIT.java b/server/src/internalClusterTest/java/org/opensearch/gateway/remote/RemoteStatePublicationIT.java index faab3645ae894..ffb9352e8ba47 100644 --- a/server/src/internalClusterTest/java/org/opensearch/gateway/remote/RemoteStatePublicationIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/gateway/remote/RemoteStatePublicationIT.java @@ -56,6 +56,7 @@ import static org.opensearch.gateway.remote.RemoteClusterStateService.REMOTE_PUBLICATION_SETTING; import static org.opensearch.gateway.remote.RemoteClusterStateService.REMOTE_PUBLICATION_SETTING_KEY; import static org.opensearch.gateway.remote.RemoteClusterStateUtils.DELIMITER; +import static org.opensearch.gateway.remote.RemoteDownloadStats.CHECKSUM_VALIDATION_FAILED_COUNT; import static org.opensearch.gateway.remote.model.RemoteClusterBlocks.CLUSTER_BLOCKS; import static org.opensearch.gateway.remote.model.RemoteCoordinationMetadata.COORDINATION_METADATA; import static org.opensearch.gateway.remote.model.RemoteCustomMetadata.CUSTOM_METADATA; @@ -405,10 +406,28 @@ private void assertDataNodeDownloadStats(NodesStatsResponse nodesStatsResponse) assertTrue(dataNodeDiscoveryStats.getClusterStateStats().getPersistenceStats().get(0).getSuccessCount() > 0); assertEquals(0, dataNodeDiscoveryStats.getClusterStateStats().getPersistenceStats().get(0).getFailedCount()); assertTrue(dataNodeDiscoveryStats.getClusterStateStats().getPersistenceStats().get(0).getTotalTimeInMillis() > 0); + assertEquals( + 0, + dataNodeDiscoveryStats.getClusterStateStats() + .getPersistenceStats() + .get(0) + .getExtendedFields() + .get(CHECKSUM_VALIDATION_FAILED_COUNT) + .get() + ); assertTrue(dataNodeDiscoveryStats.getClusterStateStats().getPersistenceStats().get(1).getSuccessCount() > 0); assertEquals(0, dataNodeDiscoveryStats.getClusterStateStats().getPersistenceStats().get(1).getFailedCount()); assertTrue(dataNodeDiscoveryStats.getClusterStateStats().getPersistenceStats().get(1).getTotalTimeInMillis() > 0); + assertEquals( + 0, + dataNodeDiscoveryStats.getClusterStateStats() + .getPersistenceStats() + .get(1) + .getExtendedFields() + .get(CHECKSUM_VALIDATION_FAILED_COUNT) + .get() + ); } private Map getMetadataFiles(BlobStoreRepository repository, String subDirectory) throws IOException { diff --git a/server/src/main/java/org/opensearch/gateway/remote/RemoteClusterStateService.java b/server/src/main/java/org/opensearch/gateway/remote/RemoteClusterStateService.java index 12d10fd908b44..ece29180f9cf5 100644 --- a/server/src/main/java/org/opensearch/gateway/remote/RemoteClusterStateService.java +++ b/server/src/main/java/org/opensearch/gateway/remote/RemoteClusterStateService.java @@ -1644,6 +1644,12 @@ void validateClusterStateFromChecksum( failedValidation ) ); + if (isFullStateDownload) { + remoteStateStats.stateFullDownloadValidationFailed(); + } else { + remoteStateStats.stateDiffDownloadValidationFailed(); + } + if (isFullStateDownload && remoteClusterStateValidationMode.equals(RemoteClusterStateValidationMode.FAILURE)) { throw new IllegalStateException( "Cluster state checksums do not match during full state read. Validation failed for " + failedValidation diff --git a/server/src/main/java/org/opensearch/gateway/remote/RemoteDownloadStats.java b/server/src/main/java/org/opensearch/gateway/remote/RemoteDownloadStats.java new file mode 100644 index 0000000000000..a8f4b33a19c37 --- /dev/null +++ b/server/src/main/java/org/opensearch/gateway/remote/RemoteDownloadStats.java @@ -0,0 +1,36 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.gateway.remote; + +import org.opensearch.cluster.coordination.PersistedStateStats; + +import java.util.concurrent.atomic.AtomicLong; + +/** + * Download stats for remote state + * + * @opensearch.internal + */ +public class RemoteDownloadStats extends PersistedStateStats { + static final String CHECKSUM_VALIDATION_FAILED_COUNT = "checksum_validation_failed_count"; + private AtomicLong checksumValidationFailedCount = new AtomicLong(0); + + public RemoteDownloadStats(String statsName) { + super(statsName); + addToExtendedFields(CHECKSUM_VALIDATION_FAILED_COUNT, checksumValidationFailedCount); + } + + public void checksumValidationFailedCount() { + checksumValidationFailedCount.incrementAndGet(); + } + + public long getChecksumValidationFailedCount() { + return checksumValidationFailedCount.get(); + } +} diff --git a/server/src/main/java/org/opensearch/gateway/remote/RemotePersistenceStats.java b/server/src/main/java/org/opensearch/gateway/remote/RemotePersistenceStats.java index 417ebdafd3ba7..11f26ac8b3ed9 100644 --- a/server/src/main/java/org/opensearch/gateway/remote/RemotePersistenceStats.java +++ b/server/src/main/java/org/opensearch/gateway/remote/RemotePersistenceStats.java @@ -18,16 +18,16 @@ public class RemotePersistenceStats { RemoteUploadStats remoteUploadStats; - PersistedStateStats remoteDiffDownloadStats; - PersistedStateStats remoteFullDownloadStats; + RemoteDownloadStats remoteDiffDownloadStats; + RemoteDownloadStats remoteFullDownloadStats; final String FULL_DOWNLOAD_STATS = "remote_full_download"; final String DIFF_DOWNLOAD_STATS = "remote_diff_download"; public RemotePersistenceStats() { remoteUploadStats = new RemoteUploadStats(); - remoteDiffDownloadStats = new PersistedStateStats(DIFF_DOWNLOAD_STATS); - remoteFullDownloadStats = new PersistedStateStats(FULL_DOWNLOAD_STATS); + remoteDiffDownloadStats = new RemoteDownloadStats(DIFF_DOWNLOAD_STATS); + remoteFullDownloadStats = new RemoteDownloadStats(FULL_DOWNLOAD_STATS); } public void cleanUpAttemptFailed() { @@ -90,6 +90,22 @@ public void stateDiffDownloadFailed() { remoteDiffDownloadStats.stateFailed(); } + public void stateDiffDownloadValidationFailed() { + remoteDiffDownloadStats.checksumValidationFailedCount(); + } + + public void stateFullDownloadValidationFailed() { + remoteFullDownloadStats.checksumValidationFailedCount(); + } + + public long getStateDiffDownloadValidationFailed() { + return remoteDiffDownloadStats.getChecksumValidationFailedCount(); + } + + public long getStateFullDownloadValidationFailed() { + return remoteFullDownloadStats.getChecksumValidationFailedCount(); + } + public PersistedStateStats getUploadStats() { return remoteUploadStats; } diff --git a/server/src/test/java/org/opensearch/gateway/remote/RemoteClusterStateServiceTests.java b/server/src/test/java/org/opensearch/gateway/remote/RemoteClusterStateServiceTests.java index b11d5e48bec06..56857285fa8d3 100644 --- a/server/src/test/java/org/opensearch/gateway/remote/RemoteClusterStateServiceTests.java +++ b/server/src/test/java/org/opensearch/gateway/remote/RemoteClusterStateServiceTests.java @@ -3342,6 +3342,7 @@ public void testGetClusterStateForManifestWithChecksumValidationEnabledWithNullC anyString(), anyBoolean() ); + assertEquals(0, remoteClusterStateService.getRemoteStateStats().getStateFullDownloadValidationFailed()); } public void testGetClusterStateForManifestWithChecksumValidationEnabled() throws IOException { @@ -3374,6 +3375,7 @@ public void testGetClusterStateForManifestWithChecksumValidationEnabled() throws ); mockService.getClusterStateForManifest(ClusterName.DEFAULT.value(), manifest, NODE_ID, true); verify(mockService, times(1)).validateClusterStateFromChecksum(manifest, clusterState, ClusterName.DEFAULT.value(), NODE_ID, true); + assertEquals(0, remoteClusterStateService.getRemoteStateStats().getStateFullDownloadValidationFailed()); } public void testGetClusterStateForManifestWithChecksumValidationModeNone() throws IOException { @@ -3406,6 +3408,7 @@ public void testGetClusterStateForManifestWithChecksumValidationModeNone() throw ); mockService.getClusterStateForManifest(ClusterName.DEFAULT.value(), manifest, NODE_ID, true); verify(mockService, times(0)).validateClusterStateFromChecksum(any(), any(), anyString(), anyString(), anyBoolean()); + assertEquals(0, remoteClusterStateService.getRemoteStateStats().getStateFullDownloadValidationFailed()); } public void testGetClusterStateForManifestWithChecksumValidationEnabledWithMismatch() throws IOException { @@ -3448,6 +3451,7 @@ public void testGetClusterStateForManifestWithChecksumValidationEnabledWithMisma NODE_ID, true ); + assertEquals(1, remoteClusterStateService.getRemoteStateStats().getStateFullDownloadValidationFailed()); } public void testGetClusterStateForManifestWithChecksumValidationDebugWithMismatch() throws IOException { @@ -3494,6 +3498,7 @@ public void testGetClusterStateForManifestWithChecksumValidationDebugWithMismatc NODE_ID, true ); + assertEquals(1, remoteClusterStateService.getRemoteStateStats().getStateFullDownloadValidationFailed()); } public void testGetClusterStateUsingDiffWithChecksum() throws IOException { @@ -3535,6 +3540,7 @@ public void testGetClusterStateUsingDiffWithChecksum() throws IOException { eq(NODE_ID), eq(false) ); + assertEquals(0, remoteClusterStateService.getRemoteStateStats().getStateDiffDownloadValidationFailed()); } public void testGetClusterStateUsingDiffWithChecksumModeNone() throws IOException { @@ -3576,6 +3582,7 @@ public void testGetClusterStateUsingDiffWithChecksumModeNone() throws IOExceptio eq(NODE_ID), eq(false) ); + assertEquals(0, remoteClusterStateService.getRemoteStateStats().getStateDiffDownloadValidationFailed()); } public void testGetClusterStateUsingDiffWithChecksumModeDebugMismatch() throws IOException { @@ -3616,6 +3623,7 @@ public void testGetClusterStateUsingDiffWithChecksumModeDebugMismatch() throws I eq(NODE_ID), eq(false) ); + assertEquals(1, remoteClusterStateService.getRemoteStateStats().getStateDiffDownloadValidationFailed()); } public void testGetClusterStateUsingDiffWithChecksumModeTraceMismatch() throws IOException { @@ -3677,6 +3685,7 @@ public void testGetClusterStateUsingDiffWithChecksumModeTraceMismatch() throws I eq(NODE_ID), eq(false) ); + assertEquals(1, remoteClusterStateService.getRemoteStateStats().getStateDiffDownloadValidationFailed()); } public void testGetClusterStateUsingDiffWithChecksumMismatch() throws IOException { @@ -3738,6 +3747,7 @@ public void testGetClusterStateUsingDiffWithChecksumMismatch() throws IOExceptio eq(NODE_ID), eq(false) ); + assertEquals(1, remoteClusterStateService.getRemoteStateStats().getStateDiffDownloadValidationFailed()); } private void mockObjectsForGettingPreviousClusterUUID(Map clusterUUIDsPointers) throws IOException { From 8347d0ec22aa865dea3bc4b2027fb7bdf486fab2 Mon Sep 17 00:00:00 2001 From: gaobinlong Date: Wed, 18 Sep 2024 03:12:05 +0800 Subject: [PATCH 4/6] Add validation for the search backpressure cancellation settings (#15501) * Fix updating search backpressure settings crashing the cluster Signed-off-by: Gao Binlong * Modify change log Signed-off-by: Gao Binlong * Fix version check Signed-off-by: Gao Binlong * Increase test coverage Signed-off-by: Gao Binlong * Format the code Signed-off-by: Gao Binlong * Optimize some code Signed-off-by: Gao Binlong * Fix typo Signed-off-by: Gao Binlong --------- Signed-off-by: Gao Binlong --- CHANGELOG.md | 2 + .../test/cluster.put_settings/10_basic.yml | 113 ++++++++++++++++++ .../opensearch/common/settings/Setting.java | 13 ++ .../SearchBackpressureService.java | 6 +- .../backpressure/SearchBackpressureState.java | 5 +- .../settings/SearchBackpressureSettings.java | 16 ++- .../settings/SearchShardTaskSettings.java | 16 ++- .../settings/SearchTaskSettings.java | 16 ++- .../common/settings/SettingTests.java | 28 +++++ .../SearchBackpressureSettingsTests.java | 28 +++++ 10 files changed, 231 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cc313fdcdb849..db41e2eee16f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Fixed - Fix wildcard query containing escaped character ([#15737](https://github.com/opensearch-project/OpenSearch/pull/15737)) - Fix case-insensitive query on wildcard field ([#15882](https://github.com/opensearch-project/OpenSearch/pull/15882)) +- Add validation for the search backpressure cancellation settings ([#15501](https://github.com/opensearch-project/OpenSearch/pull/15501)) + ### Security [Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.17...2.x diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.put_settings/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.put_settings/10_basic.yml index 107d298b597d3..0f4a45e4591a3 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.put_settings/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.put_settings/10_basic.yml @@ -98,3 +98,116 @@ - match: {error.root_cause.0.type: "illegal_argument_exception"} - match: { error.root_cause.0.reason: "Invalid SearchBackpressureMode: monitor-only" } - match: { status: 400 } + + +--- +"Test setting search backpressure cancellation settings": + - skip: + version: "- 2.99.99" + reason: "Fixed in 3.0.0" + + - do: + cluster.put_settings: + body: + transient: + search_backpressure.search_task.cancellation_burst: 11 + - is_true: acknowledged + + - do: + cluster.get_settings: + flat_settings: false + - match: {transient.search_backpressure.search_task.cancellation_burst: "11"} + + - do: + cluster.put_settings: + body: + transient: + search_backpressure.search_task.cancellation_rate: 0.1 + - is_true: acknowledged + + - do: + cluster.get_settings: + flat_settings: false + - match: {transient.search_backpressure.search_task.cancellation_rate: "0.1"} + + - do: + cluster.put_settings: + body: + transient: + search_backpressure.search_task.cancellation_ratio: 0.2 + - is_true: acknowledged + + - do: + cluster.get_settings: + flat_settings: false + - match: {transient.search_backpressure.search_task.cancellation_ratio: "0.2"} + + - do: + cluster.put_settings: + body: + transient: + search_backpressure.search_shard_task.cancellation_burst: 12 + - is_true: acknowledged + + - do: + cluster.get_settings: + flat_settings: false + - match: {transient.search_backpressure.search_shard_task.cancellation_burst: "12"} + + - do: + cluster.put_settings: + body: + transient: + search_backpressure.search_shard_task.cancellation_rate: 0.3 + - is_true: acknowledged + + - do: + cluster.get_settings: + flat_settings: false + - match: {transient.search_backpressure.search_shard_task.cancellation_rate: "0.3"} + + - do: + cluster.put_settings: + body: + transient: + search_backpressure.search_shard_task.cancellation_ratio: 0.4 + - is_true: acknowledged + + - do: + cluster.get_settings: + flat_settings: false + - match: {transient.search_backpressure.search_shard_task.cancellation_ratio: "0.4"} + +--- +"Test setting invalid search backpressure cancellation_rate and cancellation_ratio": + - skip: + version: "- 2.99.99" + reason: "Fixed in 3.0.0" + + - do: + catch: /search_backpressure.search_task.cancellation_rate must be > 0/ + cluster.put_settings: + body: + transient: + search_backpressure.search_task.cancellation_rate: 0.0 + + - do: + catch: /search_backpressure.search_task.cancellation_ratio must be > 0/ + cluster.put_settings: + body: + transient: + search_backpressure.search_task.cancellation_ratio: 0.0 + + - do: + catch: /search_backpressure.search_shard_task.cancellation_rate must be > 0/ + cluster.put_settings: + body: + transient: + search_backpressure.search_shard_task.cancellation_rate: 0.0 + + - do: + catch: /search_backpressure.search_shard_task.cancellation_ratio must be > 0/ + cluster.put_settings: + body: + transient: + search_backpressure.search_shard_task.cancellation_ratio: 0.0 diff --git a/server/src/main/java/org/opensearch/common/settings/Setting.java b/server/src/main/java/org/opensearch/common/settings/Setting.java index fea4c165809ba..081029c1c106c 100644 --- a/server/src/main/java/org/opensearch/common/settings/Setting.java +++ b/server/src/main/java/org/opensearch/common/settings/Setting.java @@ -1855,6 +1855,10 @@ public static Setting doubleSetting( ); } + public static Setting doubleSetting(String key, double defaultValue, Validator validator, Property... properties) { + return new Setting<>(key, Double.toString(defaultValue), Double::parseDouble, validator, properties); + } + /** * A writeable parser for double * @@ -1961,6 +1965,15 @@ public static Setting doubleSetting( ); } + public static Setting doubleSetting( + String key, + Setting fallbackSetting, + Validator validator, + Property... properties + ) { + return new Setting<>(new SimpleKey(key), fallbackSetting, fallbackSetting::getRaw, Double::parseDouble, validator, properties); + } + /// simpleString public static Setting simpleString(String key, Property... properties) { diff --git a/server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureService.java b/server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureService.java index 43b9f8ae87529..e98046ba1dede 100644 --- a/server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureService.java +++ b/server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureService.java @@ -158,14 +158,16 @@ public SearchBackpressureService( timeNanosSupplier, getSettings().getSearchTaskSettings().getCancellationRateNanos(), getSettings().getSearchTaskSettings().getCancellationBurst(), - getSettings().getSearchTaskSettings().getCancellationRatio() + getSettings().getSearchTaskSettings().getCancellationRatio(), + getSettings().getSearchTaskSettings().getCancellationRate() ), SearchShardTask.class, new SearchBackpressureState( timeNanosSupplier, getSettings().getSearchShardTaskSettings().getCancellationRateNanos(), getSettings().getSearchShardTaskSettings().getCancellationBurst(), - getSettings().getSearchShardTaskSettings().getCancellationRatio() + getSettings().getSearchShardTaskSettings().getCancellationRatio(), + getSettings().getSearchShardTaskSettings().getCancellationRate() ) ); this.settings.getSearchTaskSettings().addListener(searchBackpressureStates.get(SearchTask.class)); diff --git a/server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureState.java b/server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureState.java index 5f086bd498036..36f5b25e002c3 100644 --- a/server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureState.java +++ b/server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureState.java @@ -43,12 +43,15 @@ public class SearchBackpressureState implements CancellationSettingsListener { LongSupplier timeNanosSupplier, double cancellationRateNanos, double cancellationBurst, - double cancellationRatio + double cancellationRatio, + double cancellationRate ) { rateLimiter = new AtomicReference<>(new TokenBucket(timeNanosSupplier, cancellationRateNanos, cancellationBurst)); ratioLimiter = new AtomicReference<>(new TokenBucket(this::getCompletionCount, cancellationRatio, cancellationBurst)); this.timeNanosSupplier = timeNanosSupplier; this.cancellationBurst = cancellationBurst; + this.cancellationRatio = cancellationRatio; + this.cancellationRate = cancellationRate; } public long getCompletionCount() { diff --git a/server/src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java b/server/src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java index 79494eb0d3c24..55a031382f282 100644 --- a/server/src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java +++ b/server/src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java @@ -61,8 +61,14 @@ private static class Defaults { public static final Setting SETTING_CANCELLATION_RATIO = Setting.doubleSetting( "search_backpressure.cancellation_ratio", Defaults.CANCELLATION_RATIO, - 0.0, - 1.0, + value -> { + if (value <= 0.0) { + throw new IllegalArgumentException("search_backpressure.cancellation_ratio must be > 0"); + } + if (value > 1.0) { + throw new IllegalArgumentException("search_backpressure.cancellation_ratio must be <= 1.0"); + } + }, Setting.Property.Deprecated, Setting.Property.Dynamic, Setting.Property.NodeScope @@ -78,7 +84,11 @@ private static class Defaults { public static final Setting SETTING_CANCELLATION_RATE = Setting.doubleSetting( "search_backpressure.cancellation_rate", Defaults.CANCELLATION_RATE, - 0.0, + value -> { + if (value <= 0.0) { + throw new IllegalArgumentException("search_backpressure.cancellation_rate must be > 0"); + } + }, Setting.Property.Deprecated, Setting.Property.Dynamic, Setting.Property.NodeScope diff --git a/server/src/main/java/org/opensearch/search/backpressure/settings/SearchShardTaskSettings.java b/server/src/main/java/org/opensearch/search/backpressure/settings/SearchShardTaskSettings.java index 6d016c7466362..38213506c55b7 100644 --- a/server/src/main/java/org/opensearch/search/backpressure/settings/SearchShardTaskSettings.java +++ b/server/src/main/java/org/opensearch/search/backpressure/settings/SearchShardTaskSettings.java @@ -44,8 +44,14 @@ private static class Defaults { public static final Setting SETTING_CANCELLATION_RATIO = Setting.doubleSetting( "search_backpressure.search_shard_task.cancellation_ratio", SearchBackpressureSettings.SETTING_CANCELLATION_RATIO, - 0.0, - 1.0, + value -> { + if (value <= 0.0) { + throw new IllegalArgumentException("search_backpressure.search_shard_task.cancellation_ratio must be > 0"); + } + if (value > 1.0) { + throw new IllegalArgumentException("search_backpressure.search_shard_task.cancellation_ratio must be <= 1.0"); + } + }, Setting.Property.Dynamic, Setting.Property.NodeScope ); @@ -58,7 +64,11 @@ private static class Defaults { public static final Setting SETTING_CANCELLATION_RATE = Setting.doubleSetting( "search_backpressure.search_shard_task.cancellation_rate", SearchBackpressureSettings.SETTING_CANCELLATION_RATE, - 0.0, + value -> { + if (value <= 0.0) { + throw new IllegalArgumentException("search_backpressure.search_shard_task.cancellation_rate must be > 0"); + } + }, Setting.Property.Dynamic, Setting.Property.NodeScope ); diff --git a/server/src/main/java/org/opensearch/search/backpressure/settings/SearchTaskSettings.java b/server/src/main/java/org/opensearch/search/backpressure/settings/SearchTaskSettings.java index 4b34323b1ddc6..f9af7f9b59fdb 100644 --- a/server/src/main/java/org/opensearch/search/backpressure/settings/SearchTaskSettings.java +++ b/server/src/main/java/org/opensearch/search/backpressure/settings/SearchTaskSettings.java @@ -48,8 +48,14 @@ private static class Defaults { public static final Setting SETTING_CANCELLATION_RATIO = Setting.doubleSetting( "search_backpressure.search_task.cancellation_ratio", Defaults.CANCELLATION_RATIO, - 0.0, - 1.0, + value -> { + if (value <= 0.0) { + throw new IllegalArgumentException("search_backpressure.search_task.cancellation_ratio must be > 0"); + } + if (value > 1.0) { + throw new IllegalArgumentException("search_backpressure.search_task.cancellation_ratio must be <= 1.0"); + } + }, Setting.Property.Dynamic, Setting.Property.NodeScope ); @@ -62,7 +68,11 @@ private static class Defaults { public static final Setting SETTING_CANCELLATION_RATE = Setting.doubleSetting( "search_backpressure.search_task.cancellation_rate", Defaults.CANCELLATION_RATE, - 0.0, + value -> { + if (value <= 0.0) { + throw new IllegalArgumentException("search_backpressure.search_task.cancellation_rate must be > 0"); + } + }, Setting.Property.Dynamic, Setting.Property.NodeScope ); diff --git a/server/src/test/java/org/opensearch/common/settings/SettingTests.java b/server/src/test/java/org/opensearch/common/settings/SettingTests.java index 7ebee680e8e52..c3c399a9d88b2 100644 --- a/server/src/test/java/org/opensearch/common/settings/SettingTests.java +++ b/server/src/test/java/org/opensearch/common/settings/SettingTests.java @@ -1274,6 +1274,20 @@ public void testFloatParser() throws Exception { public void testDoubleWithDefaultValue() { Setting doubleSetting = Setting.doubleSetting("foo.bar", 42.1); assertEquals(doubleSetting.get(Settings.EMPTY), Double.valueOf(42.1)); + + Setting doubleSettingWithValidator = Setting.doubleSetting("foo.bar", 42.1, value -> { + if (value <= 0.0) { + throw new IllegalArgumentException("The setting foo.bar must be >0"); + } + }); + try { + assertThrows( + IllegalArgumentException.class, + () -> doubleSettingWithValidator.get(Settings.builder().put("foo.bar", randomFrom(-1, 0)).build()) + ); + } catch (IllegalArgumentException ex) { + assertEquals("The setting foo.bar must be >0", ex.getMessage()); + } } public void testDoubleWithFallbackValue() { @@ -1282,6 +1296,20 @@ public void testDoubleWithFallbackValue() { assertEquals(doubleSetting.get(Settings.EMPTY), Double.valueOf(2.1)); assertEquals(doubleSetting.get(Settings.builder().put("foo.bar", 3.2).build()), Double.valueOf(3.2)); assertEquals(doubleSetting.get(Settings.builder().put("foo.baz", 3.2).build()), Double.valueOf(3.2)); + + Setting doubleSettingWithValidator = Setting.doubleSetting("foo.bar", fallbackSetting, value -> { + if (value <= 0.0) { + throw new IllegalArgumentException("The setting foo.bar must be >0"); + } + }); + try { + assertThrows( + IllegalArgumentException.class, + () -> doubleSettingWithValidator.get(Settings.builder().put("foo.bar", randomFrom(-1, 0)).build()) + ); + } catch (IllegalArgumentException ex) { + assertEquals("The setting foo.bar must be >0", ex.getMessage()); + } } public void testDoubleWithMinMax() throws Exception { diff --git a/server/src/test/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java b/server/src/test/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java index a02ca3cf877ad..683ada76c7683 100644 --- a/server/src/test/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java +++ b/server/src/test/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java @@ -37,4 +37,32 @@ public void testSearchBackpressureSettingValidateInvalidMode() { () -> new SearchBackpressureSettings(settings, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)) ); } + + public void testInvalidCancellationRate() { + Settings settings1 = Settings.builder().put("search_backpressure.search_task.cancellation_rate", randomFrom(-1, 0)).build(); + assertThrows( + IllegalArgumentException.class, + () -> new SearchBackpressureSettings(settings1, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)) + ); + + Settings settings2 = Settings.builder().put("search_backpressure.search_shard_task.cancellation_rate", randomFrom(-1, 0)).build(); + assertThrows( + IllegalArgumentException.class, + () -> new SearchBackpressureSettings(settings2, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)) + ); + } + + public void testInvalidCancellationRatio() { + Settings settings1 = Settings.builder().put("search_backpressure.search_task.cancellation_ratio", randomFrom(-1, 0)).build(); + assertThrows( + IllegalArgumentException.class, + () -> new SearchBackpressureSettings(settings1, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)) + ); + + Settings settings2 = Settings.builder().put("search_backpressure.search_shard_task.cancellation_ratio", randomFrom(-1, 0)).build(); + assertThrows( + IllegalArgumentException.class, + () -> new SearchBackpressureSettings(settings2, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)) + ); + } } From eb5b70370e548cd0f3b51721397df818b3050daa Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Tue, 17 Sep 2024 15:44:32 -0400 Subject: [PATCH 5/6] Fix flaky terminaton conditions for org.opensearch.rest.ReactorNetty4StreamingStressIT.testCloseClientStreamingRequest test case (#15959) Signed-off-by: Andriy Redko --- .../org/opensearch/rest/ReactorNetty4StreamingStressIT.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/plugins/transport-reactor-netty4/src/javaRestTest/java/org/opensearch/rest/ReactorNetty4StreamingStressIT.java b/plugins/transport-reactor-netty4/src/javaRestTest/java/org/opensearch/rest/ReactorNetty4StreamingStressIT.java index a853ed56fad32..9da456f618ffc 100644 --- a/plugins/transport-reactor-netty4/src/javaRestTest/java/org/opensearch/rest/ReactorNetty4StreamingStressIT.java +++ b/plugins/transport-reactor-netty4/src/javaRestTest/java/org/opensearch/rest/ReactorNetty4StreamingStressIT.java @@ -8,6 +8,7 @@ package org.opensearch.rest; +import org.apache.hc.core5.http.ConnectionClosedException; import org.opensearch.client.Request; import org.opensearch.client.Response; import org.opensearch.client.StreamingRequest; @@ -75,7 +76,7 @@ public void testCloseClientStreamingRequest() throws Exception { } }) .then(() -> scheduler.advanceTimeBy(delay)) - .expectErrorMatches(t -> t instanceof InterruptedIOException) + .expectErrorMatches(t -> t instanceof InterruptedIOException || t instanceof ConnectionClosedException) .verify(); } } From 7c427d9fd588597343654d2f534952f7116d1df4 Mon Sep 17 00:00:00 2001 From: David Zane <38449481+dzane17@users.noreply.github.com> Date: Tue, 17 Sep 2024 15:29:12 -0700 Subject: [PATCH 6/6] Implement WithFieldName interface in ValuesSourceAggregationBuilder & FieldSortBuilder (#15916) Signed-off-by: David Zane --- CHANGELOG.md | 1 + .../support/ValuesSourceAggregationBuilder.java | 10 +++++++++- .../org/opensearch/search/sort/FieldSortBuilder.java | 8 +++++++- .../bucket/range/RangeAggregationBuilderTests.java | 1 + .../opensearch/search/sort/FieldSortBuilderTests.java | 1 + 5 files changed, 19 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index db41e2eee16f5..309d9cb06cc94 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - MultiTermQueries in keyword fields now default to `indexed` approach and gated behind cluster setting ([#15637](https://github.com/opensearch-project/OpenSearch/pull/15637)) - [Workload Management] QueryGroup resource cancellation framework changes ([#15651](https://github.com/opensearch-project/OpenSearch/pull/15651)) - Fallback to Remote cluster-state on Term-Version check mismatch - ([#15424](https://github.com/opensearch-project/OpenSearch/pull/15424)) +- Implement WithFieldName interface in ValuesSourceAggregationBuilder & FieldSortBuilder ([#15916](https://github.com/opensearch-project/OpenSearch/pull/15916)) ### Dependencies - Bump `com.azure:azure-identity` from 1.13.0 to 1.13.2 ([#15578](https://github.com/opensearch-project/OpenSearch/pull/15578)) diff --git a/server/src/main/java/org/opensearch/search/aggregations/support/ValuesSourceAggregationBuilder.java b/server/src/main/java/org/opensearch/search/aggregations/support/ValuesSourceAggregationBuilder.java index 7a73fafb4a809..1ccceb1d77dcb 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/support/ValuesSourceAggregationBuilder.java +++ b/server/src/main/java/org/opensearch/search/aggregations/support/ValuesSourceAggregationBuilder.java @@ -40,6 +40,7 @@ import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; import org.opensearch.index.query.QueryShardContext; +import org.opensearch.index.query.WithFieldName; import org.opensearch.script.Script; import org.opensearch.search.aggregations.AbstractAggregationBuilder; import org.opensearch.search.aggregations.AggregationInitializationException; @@ -57,7 +58,9 @@ * * @opensearch.internal */ -public abstract class ValuesSourceAggregationBuilder> extends AbstractAggregationBuilder { +public abstract class ValuesSourceAggregationBuilder> extends AbstractAggregationBuilder + implements + WithFieldName { public static void declareFields( AbstractObjectParser, T> objectParser, @@ -292,6 +295,11 @@ public String field() { return field; } + @Override + public String fieldName() { + return field(); + } + /** * Sets the script to use for this aggregation. */ diff --git a/server/src/main/java/org/opensearch/search/sort/FieldSortBuilder.java b/server/src/main/java/org/opensearch/search/sort/FieldSortBuilder.java index 5cecda1346b90..9825b2cbbe08e 100644 --- a/server/src/main/java/org/opensearch/search/sort/FieldSortBuilder.java +++ b/server/src/main/java/org/opensearch/search/sort/FieldSortBuilder.java @@ -65,6 +65,7 @@ import org.opensearch.index.query.QueryRewriteContext; import org.opensearch.index.query.QueryShardContext; import org.opensearch.index.query.QueryShardException; +import org.opensearch.index.query.WithFieldName; import org.opensearch.search.DocValueFormat; import org.opensearch.search.MultiValueMode; import org.opensearch.search.SearchSortValuesAndFormats; @@ -86,7 +87,7 @@ * * @opensearch.internal */ -public class FieldSortBuilder extends SortBuilder { +public class FieldSortBuilder extends SortBuilder implements WithFieldName { private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(FieldSortBuilder.class); public static final String NAME = "field_sort"; @@ -184,6 +185,11 @@ public String getFieldName() { return this.fieldName; } + @Override + public String fieldName() { + return getFieldName(); + } + /** * Sets the value when a field is missing in a doc. Can also be set to {@code _last} or * {@code _first} to sort missing last or first respectively. diff --git a/server/src/test/java/org/opensearch/search/aggregations/bucket/range/RangeAggregationBuilderTests.java b/server/src/test/java/org/opensearch/search/aggregations/bucket/range/RangeAggregationBuilderTests.java index 4362ce48003cc..14532e30f8984 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/bucket/range/RangeAggregationBuilderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/bucket/range/RangeAggregationBuilderTests.java @@ -128,6 +128,7 @@ public void testNumericKeys() throws IOException { ); assertThat(builder.getName(), equalTo("test")); assertThat(builder.field(), equalTo("f")); + assertThat(builder.fieldName(), equalTo("f")); assertThat(builder.ranges, equalTo(List.of(new RangeAggregator.Range("1", null, 0d)))); } } diff --git a/server/src/test/java/org/opensearch/search/sort/FieldSortBuilderTests.java b/server/src/test/java/org/opensearch/search/sort/FieldSortBuilderTests.java index 9b8cd1b5f1ce0..ced952db555aa 100644 --- a/server/src/test/java/org/opensearch/search/sort/FieldSortBuilderTests.java +++ b/server/src/test/java/org/opensearch/search/sort/FieldSortBuilderTests.java @@ -196,6 +196,7 @@ protected void sortFieldAssertions(FieldSortBuilder builder, SortField sortField assertEquals(builder.order() == SortOrder.ASC ? false : true, sortField.getReverse()); if (expectedType == SortField.Type.CUSTOM) { assertEquals(builder.getFieldName(), sortField.getField()); + assertEquals(builder.fieldName(), sortField.getField()); } assertEquals(DocValueFormat.RAW, format); }