Skip to content

Allow starting Elasticsearch container with VM options in query runner#11405

Merged
ebyhr merged 1 commit intotrinodb:masterfrom
ebyhr:ebi/elasticsearch-test-containers
Mar 19, 2022
Merged

Allow starting Elasticsearch container with VM options in query runner#11405
ebyhr merged 1 commit intotrinodb:masterfrom
ebyhr:ebi/elasticsearch-test-containers

Conversation

@ebyhr
Copy link
Copy Markdown
Member

@ebyhr ebyhr commented Mar 10, 2022

Description

Allow starting Elasticsearch container with VM options in query runner

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

@ebyhr ebyhr added the maintenance Project maintenance task label Mar 10, 2022
@cla-bot cla-bot bot added the cla-signed label Mar 10, 2022
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be a proper argument to the query runner, not a magic system property

Copy link
Copy Markdown
Member Author

@ebyhr ebyhr Mar 10, 2022

Choose a reason for hiding this comment

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

a proper argument to the query runner"

Do you mean passing as String[] args in the main method? The reason I used a system property is #3322 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How is the server lifecycle managed? How does it get stopped?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The underlying container would be managed by Testcontainers ryuk. It will be closed with some delay when stopping the query runner.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We have some rudimentary support for "container reuse", which i think it what you're looking for.

I propose you use that, and let's avoid adding a yet new method of skipping container creation.

BTW, for manual testing, I find PT envs very useful. ptl env up .. --without-trino + DevelopmentServer is what i use locally. This requires no changes in the query runners. Of course, this works for manual testing, and doesn't work for running integration tests (those using QueryRunner), but i understand this is the use-case you're addressing here.

cc @kokosing @losipiuk

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

BTW starting ElasticsearchServer should actually be the default (and maybe the only) behavior of ElasticsearchQueryRunner::main

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don’t think so. When I wrote that class and method, the goal was to be able to run Trino against an external ES instance so that I could try multiple different ES configurations (single node, a cluster, AWS ES, etc) as I was developing it. Hardcoding it defeats that purpose.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@martint i see your point, but that's not how QueryRunners.main are generally implemented.
They are self-contained first and foremost, and then re-usable if needed. I don't think ES should be special here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

How about adding another query runner for external service usage? Then, we can use ElasticsearchQueryRunner for dockerized environment.

public final class ElasticsearchExternalQueryRunner
{
    private ElasticsearchExternalQueryRunner() {}

    public static void main(String[] args)
            throws Exception
    {
        Logging.initialize();

        DistributedQueryRunner queryRunner = createElasticsearchQueryRunner(
                HostAndPort.fromParts("localhost", 9200),
                TpchTable.getTables(),
                ImmutableMap.of("http-server.http.port", "8080"),
                ImmutableMap.of(),
                3);

        Logger log = Logger.get(ElasticsearchExternalQueryRunner.class);
        log.info("======== SERVER STARTED ========");
        log.info("\n====\n%s\n====", queryRunner.getCoordinator().getBaseUrl());
    }
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ebyhr that sounds like a reasonable compromise.
in the ElasticsearchExternalQueryRunner please include a comment intructing hwo to start this external thing

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@findepi @martint Added another query runner.

@ebyhr ebyhr added the no-release-notes This pull request does not require release notes entry label Mar 17, 2022
@ebyhr ebyhr force-pushed the ebi/elasticsearch-test-containers branch from 137ebc8 to 146422d Compare March 18, 2022 09:26
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use same version as one used in TestElasticsearch7ConnectorTest (define a constant)

Also, add ElasticsearchExternalQueryRunner to allow
connecting to existing Elasticserver instances and
define a constant for version 7 image name.
@ebyhr ebyhr force-pushed the ebi/elasticsearch-test-containers branch from 146422d to f08c6b0 Compare March 18, 2022 13:09
@ebyhr ebyhr merged commit 42974f7 into trinodb:master Mar 19, 2022
@ebyhr ebyhr deleted the ebi/elasticsearch-test-containers branch March 19, 2022 00:15
@github-actions github-actions bot added this to the 375 milestone Mar 19, 2022
@ebyhr
Copy link
Copy Markdown
Member Author

ebyhr commented Mar 19, 2022

Merged.

@martint You need to change IntelliJ run setting to use ElasticsearchExternalQueryRunner now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed maintenance Project maintenance task no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

3 participants