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

Disable timeout params for deleteIndex API only for serverless #646

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

ykmr1224
Copy link
Collaborator

@ykmr1224 ykmr1224 commented Sep 11, 2024

Description

  • Disable timeout params for deleteIndex API only for serverless
  • OpenSearch serverless does not accept timeout / master_timeout (= clusterManagerNodeTimeout) params, which is added by the RestHighLevelClient by default. (current version of opensearch-java library won't add it automatically, but we are still using 2.6.0 (ref)

Issues Resolved

#634

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@ykmr1224 ykmr1224 marked this pull request as ready for review September 11, 2024 20:44
Copy link
Collaborator

@noCharger noCharger left a comment

Choose a reason for hiding this comment

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

non-blocking: can we handle this in RestHighLevelClientWrapper and consider use DeleteIndexRequestBuilder?

@ykmr1224
Copy link
Collaborator Author

non-blocking: can we handle this in RestHighLevelClientWrapper and consider use DeleteIndexRequestBuilder?

RestHighLevelClientWrapper does not have reference to FlintOptions.
What do you mean by DeleteIndexRequestBuilder?

@noCharger
Copy link
Collaborator

non-blocking: can we handle this in RestHighLevelClientWrapper and consider use DeleteIndexRequestBuilder?

RestHighLevelClientWrapper does not have reference to FlintOptions. What do you mean by DeleteIndexRequestBuilder?

Example:

public void deleteIndex(String indexName) {
    LOG.info("Deleting Flint index " + indexName);
    String osIndexName = sanitizeIndexName(indexName);
    try (IRestHighLevelClient client = createClient()) {
        DeleteIndexRequest request = buildDeleteIndexRequest(osIndexName);
        client.indices().delete(request, RequestOptions.DEFAULT);
    } catch (Exception e) {
        throw new IllegalStateException("Failed to delete Flint index " + osIndexName, e);
    }
}

private DeleteIndexRequest buildDeleteIndexRequest(String indexName) {
    DeleteIndexRequestBuilder builder = new DeleteIndexRequestBuilder(indexName);

    if (FlintOptions.SERVICE_NAME_AOSS.equals(options.getServiceName())) {
        // Disable timeouts for serverless environments
        builder.setClusterManagerNodeTimeout((TimeValue) null)
               .setTimeout((TimeValue) null);
    }

    return builder.build();
}

Copy link
Collaborator

@seankao-az seankao-az left a comment

Choose a reason for hiding this comment

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

lgtm
+1 on Louis' idea on moving new DeleteIndexRequest(osIndexName) into build method.
but don't think it's necessary

@ykmr1224
Copy link
Collaborator Author

ykmr1224 commented Sep 11, 2024

I think current approach is more explicit and easier to read, though there could be some preference.
btw, DeleteIndexRequestBuilder does not look like usable considering the constructor signature: public DeleteIndexRequestBuilder(OpenSearchClient client, DeleteIndexAction action, String... indices)

@noCharger noCharger merged commit 06c8c66 into opensearch-project:main Sep 11, 2024
4 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 11, 2024
Signed-off-by: Tomoyuki Morita <[email protected]>
(cherry picked from commit 06c8c66)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 11, 2024
Signed-off-by: Tomoyuki Morita <[email protected]>
(cherry picked from commit 06c8c66)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@noCharger
Copy link
Collaborator

I think current approach is more explicit and easier to read, though there could be some preference. btw, DeleteIndexRequestBuilder does not look like usable considering the constructor signature: public DeleteIndexRequestBuilder(OpenSearchClient client, DeleteIndexAction action, String... indices)

Actually, the DeleteIndexRequestBuilder from the library does not appear to be for external use; using the builder pattern externally is possible by extending it, but it is fine to leave it as is.

noCharger pushed a commit that referenced this pull request Sep 11, 2024
…#648)

(cherry picked from commit 06c8c66)

Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
dai-chen pushed a commit that referenced this pull request Sep 12, 2024
…#649)

(cherry picked from commit 06c8c66)

Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants