Skip to content

Add searchable snapshots integration tests for URL repositories #70709

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

Merged
merged 11 commits into from
Mar 26, 2021

Conversation

fcofdez
Copy link
Contributor

@fcofdez fcofdez commented Mar 23, 2021

This commit also increases the max number of pooled HTTP connections

@fcofdez fcofdez added Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 v7.13.0 >test Issues or PRs that are addressing/adding tests labels Mar 23, 2021
@fcofdez fcofdez marked this pull request as ready for review March 23, 2021 11:31
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@fcofdez fcofdez requested a review from ywelsch March 23, 2021 11:31
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I've left one suggestion which I think will give us more confidence that the client is behaving well.

@@ -18,13 +18,15 @@
import java.nio.file.Paths;
import java.util.HashMap;
import java.util.Map;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

/**
* This {@link URLFixture} exposes a filesystem directory over HTTP. It is used in repository-url
* integration tests to expose a directory created by a regular FS repository.
*/
public class URLFixture extends AbstractHttpFixture {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a fixture. I was wondering if we could plug in Nginx or some other simple http server to really make sure it works with at least a proper http server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea, I'll add a real http server to the test suite 👍

@fcofdez fcofdez requested a review from ywelsch March 25, 2021 15:51
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

I've left two build-related comments. I think someone from @elastic/es-delivery should have a look here too.

}

for (repoDir in project.ext.fsRepositoryDirs) {
delete(repoDir.value as File)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's odd that we would delete / recreate the directory at configuration time of the build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a clean environment between test runs, I couldn't find a way to run the nginx container with a directory that's created on runtime. Maybe someone from @elastic/es-delivery can help us with that?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can delete the directory contents in preprocessfixture task if necessary. We definitely don't want to do this during configuration time.

setting 'xpack.searchable.snapshot.shared_cache.region_size', '256KB'
}

tasks.register("urlThirdPartyTest") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a third-party test (it's running locally on the same machine) so why have this task?

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, that was a mistake.

@fcofdez
Copy link
Contributor Author

fcofdez commented Mar 25, 2021

@elasticmachine run elasticsearch-ci/eql-correctness
There was a network timeout unrelated to the changes

@fcofdez
Copy link
Contributor Author

fcofdez commented Mar 26, 2021

@elasticmachine update branch

@fcofdez fcofdez requested review from ywelsch and mark-vieira March 26, 2021 11:09
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for adding these tests!

@fcofdez fcofdez merged commit 3f8a925 into elastic:master Mar 26, 2021
fcofdez added a commit to fcofdez/elasticsearch that referenced this pull request Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. >test Issues or PRs that are addressing/adding tests v7.13.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants