Use Accumulo in Docker for unit tests#5598
Conversation
presto-accumulo/src/test/java/io/prestosql/plugin/accumulo/AccumuloQueryRunner.java
Outdated
Show resolved
Hide resolved
presto-accumulo/src/test/java/io/prestosql/plugin/accumulo/AccumuloQueryRunner.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Do we still need a singleton? It would be nice to have it as regular closeable resource instead.
There was a problem hiding this comment.
No, not needed. I can create a TestingAccumuloServer and refactor the tests to use it to be more similar to how the other connectors work.
There was a problem hiding this comment.
Turns out it is needed after all due to the fixed ports, multiple containers try to spawn at the same time and it fails. If I try a lock approach so only one container runs at a time, one of the test framework classes (distributed/integration smoke test) inexplicable fail because they are unable to connect to Accumulo once a new container is spawned.
I suggest with stick with the Singleton -- I'll still refactor into a TestingAccumuloServer for cleanliness.
presto-accumulo/src/test/java/io/prestosql/plugin/accumulo/AccumuloQueryRunner.java
Outdated
Show resolved
Hide resolved
presto-accumulo/src/test/java/io/prestosql/plugin/accumulo/AccumuloQueryRunner.java
Outdated
Show resolved
Hide resolved
adamjshook
force-pushed
the
accumulo-docker
branch
from
e8deaba to
82e35a0
Compare
presto-accumulo/src/test/java/io/prestosql/plugin/accumulo/AccumuloQueryRunner.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Using a singleton for now is fine as a transition from the old way, but we should move to a model where the Accumulo cluster is managed by AccumuloQueryRunner, the same way we do for other testing services. Shutting down resources immediately is useful since the CI machines can be resource constrained, and using a non-singleton allows things like testing multiple versions, testing with different config, etc.
Add a TODO here to make this not a singleton.
There was a problem hiding this comment.
but we should move to a model where the Accumulo cluster is managed by AccumuloQueryRunner,
Let's create a github issue for this and add TODO comment (with link to the issue to the code).
adamjshook
force-pushed
the
accumulo-docker
branch
from
82e35a0 to
a9467ba
Compare
|
@electrum Is this good to merge? |
There was a problem hiding this comment.
we still need to close https://github.com/prestosql/docker-images/pull/74
There was a problem hiding this comment.
It is released, pleased update to prestodev/accumulo:35
adamjshook
force-pushed
the
accumulo-docker
branch
from
a9467ba to
d921373
Compare
adamjshook
force-pushed
the
accumulo-docker
branch
from
d921373 to
86ee025
Compare
There was a problem hiding this comment.
but we should move to a model where the Accumulo cluster is managed by AccumuloQueryRunner,
Let's create a github issue for this and add TODO comment (with link to the issue to the code).
adamjshook
force-pushed
the
accumulo-docker
branch
from
86ee025 to
f7a8471
Compare
|
Merged, thanks. |
4 participants
This commit removes MiniAccumuloCluster in favor of a Docker container for Accumulo.
Depends on trinodb/docker-images#74 -- CI will fail until this is merged/released