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

couchbase: wait until query engine knows about bucket before creating… #2662

Merged
merged 1 commit into from May 29, 2020

Conversation

daschl
Copy link
Member

@daschl daschl commented May 5, 2020

… primary index

The motivation for this change is that in slow environments, it could be that the
query engine is not aware of the just created bucket yet and so the create primary
index will take longer than the 10s read timeout for the subsequent http request.

As a result, this is just another precaution to minimize the chances that in
slow environments the create primary index call times out with a read timeout.

Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

This is a good idea - thanks. Just spotted a possible resource leak; once that's resolved I'd be happy to merge.

// If the query service is enabled, make sure that we only proceed if the query engine also
// knows about the bucket in its metadata configuration.
Unreliables.retryUntilTrue(1, TimeUnit.MINUTES, () -> {
Response queryResponse = doHttpRequest(QUERY_PORT, "/query/service", "POST", new FormBody.Builder()
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought: all these Response objects ought to be closed (here and elsewhere). Otherwise we'll potentially be holding connections open on the client and server side.

This is probably much more important inside a retryUntilTrue invocation, as the frequency of retries could be high.

Lombok's @Cleanup annotation would be a cheap and easy way to ensure that these response objects get cleaned up.

Copy link
Member

Choose a reason for hiding this comment

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

@daschl ping

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry that I've been slow on this. Working on it now, will update the PR

… primary index

The motivation for this change is that in slow environments, it could be that the
query engine is not aware of the just created bucket yet and so the create primary
index will take longer than the 10s read timeout for the subsequent http request.

As a result, this is just another precaution to minimize the chances that in
slow environments the create primary index call times out with a read timeout.
Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @daschl!

@rnorth rnorth merged commit 835ac71 into testcontainers:master May 29, 2020
sd-yip added a commit to sd-yip/testcontainers-java that referenced this pull request May 12, 2021
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.

None yet

3 participants