Implement label-based container discovery for Redis Dev Service #18181

patrox · 2021-06-26T20:23:19Z

The PR started as a copy-paste solution for label-based container discovery for Redis dev service -
in order to handle: #18029.

Based on these two PRs:

As many of you have noticed this required refactoring - so for that purpose I introduced two new classes:

Both of them are located in a new module called simply: Quarkus - DevServices - Common.
The commits were not squashed (yet) for easier review.

patrox · 2021-06-26T20:31:55Z

@machi1990 @stuartwdouglas @cescoffier @Ladicek Can I please kindly ask you for a review?

machi1990

lgtm, thanks @patrox

Can you squash the commits?

docs/src/main/asciidoc/redis.adoc

extensions/devservices/common/src/main/java/io/quarkus/devservices/common/ContainerAddress.java

extensions/devservices/common/src/main/java/io/quarkus/devservices/common/ContainerLocator.java

...client/deployment/src/main/java/io/quarkus/redis/client/deployment/DevServicesProcessor.java

cescoffier · 2021-06-28T07:15:39Z

...client/deployment/src/main/java/io/quarkus/redis/client/deployment/DevServicesProcessor.java

@@ -139,7 +151,16 @@ private StartResult startContainer(String connectionName, DevServicesConfig devS

        DockerImageName dockerImageName = DockerImageName.parse(devServicesConfig.imageName.orElse(REDIS_6_ALPINE))
                .asCompatibleSubstituteFor(REDIS_6_ALPINE);
-        FixedPortRedisContainer redisContainer = new FixedPortRedisContainer(dockerImageName, devServicesConfig.port);
+
+        if (devServicesConfig.shared && launchMode == DEVELOPMENT) {


Can this be extracted too?
Maybe not easily.

I'll take a look at this as well, as it would further reduce the duplicated code.

@cescoffier I see two possibilities how this can be extracted:

in a smaller (simpler) scope we could extract the conditional logic for label-based container location (lines 155-160)

in a bit broader scope we could extract the above and also the other possibility, that is a creation of a new Container - which is done when the shared container is not used. (lines 155-172).

WDYT?

I would go with the first one.

@cescoffier I pushed the latest changes, generally, the ContainerLocater was made more "functional" by using Optional instead of bare null. This has shortened the code and additionally, it allowed to compose the method calls in a more fluent way.

cescoffier · 2021-06-28T07:16:29Z

...client/deployment/src/main/java/io/quarkus/redis/client/deployment/DevServicesProcessor.java

            super(dockerImageName);
            this.fixedExposedPort = fixedExposedPort;
+            if (serviceName != null) {
+                withLabel(DEV_SERVICE_LABEL, serviceName);


What about having a container decorator that adds the label.
At the moment we use a label-based mechanism, but this is likely going to change.

(can be done in another PR).

quarkus-bot · 2021-06-28T07:31:40Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `dc16a05`

Status	Name	Step	Test failures	Logs	Raw logs
✖	Initial JDK 11 Build	`Build`	⚠️ Check →	Logs	Raw logs

patrox · 2021-06-28T07:59:42Z

lgtm, thanks @patrox

Can you squash the commits?

Sure, they will be squashed.

Ladicek · 2021-06-28T08:08:03Z

Before I get to reviewing (which I'd really like to do per commit), what's the obsession with squashing commits? Clearly someone has made an effort to divide the work into smaller chunks that form a logical progression -- why would we want to lose that?

machi1990 · 2021-06-28T08:15:27Z

Before I get to reviewing (which I'd really like to do per commit), what's the obsession with squashing commits? Clearly someone has made an effort to divide the work into smaller chunks that form a logical progression -- why would we want to lose that?

It is not a obsession, but a suggestion. To me this boils down to two:

Implementing the label based discovery on the Redis extension using the logic from existing extension
Avoiding duplicates of the label-based discovery logic by extracting common code in a module and applying it through out.

I am all +1 on having atomic commits, that can easily be cherry-picked and have the build pass.

Ladicek · 2021-06-28T08:33:25Z

OK, I just went through the commits and the last one modifies multiple modules, whereas previously, each module was modified in a separate commit (which is very nice IMHO). In which case, squashing all the commits (except maybe the first one) would probably be best indeed. (Or percolate the changes from the last commit to each individual previous commit, though I can see how that's not the best use of one's time :-) )

patrox · 2021-06-28T10:09:02Z

OK, I just went through the commits and the last one modifies multiple modules, whereas previously, each module was modified in a separate commit (which is very nice IMHO). In which case, squashing all the commits (except maybe the first one) would probably be best indeed. (Or percolate the changes from the last commit to each individual previous commit, though I can see how that's not the best use of one's time :-) )

About the squashing process - I usually like to create a PR with separate commits to make the review a bit easier.
But, before merging it, I prefer to squash them and leave just a small number of commits that are related to separate changes.

In this case, I would leave just two commits:

Implement label-based container lookup for Redis Dev Service
Introduce a common label-based container locator + refactoring

patrox · 2021-06-28T13:14:21Z

@cescoffier @Ladicek @machi1990 I addressed all of your comments so far, can I please ask you kindly for a 2nd round?

Please note that the commits will be squashed as discussed above - to 2 separate commits.

docs/src/main/asciidoc/redis.adoc

extensions/devservices/common/src/main/java/io/quarkus/devservices/common/ContainerAddress.java

docs/src/main/asciidoc/redis-dev-services.adoc

cescoffier

A minor comment still missing (some tiny refactoring), the rest looks great.

quarkus-bot · 2021-06-29T17:22:58Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `a8d4b35`

Status	Name	Step	Test failures	Logs	Raw logs
✖	Initial JDK 11 Build	`Build`	⚠️ Check →	Logs	Raw logs

patrox · 2021-06-29T18:07:48Z

A minor comment still missing (some tiny refactoring), the rest looks great.

@cescoffier I have this change already prepared locally, I'll push it a bit later.

stuartwdouglas · 2021-07-01T03:39:59Z

docs/src/main/asciidoc/redis-dev-services.adoc

+:extension-status: preview
+include::./attributes.adoc[]
+
+Quarkus supports a feature called DevServices that allows you to create various datasources without any config.


Shouldn't this just go in the main redis doc as a subsection?

Kafka and AMQP already use this structure. The getting started contains admonitions referring to this file. Dev service configuration is more reference content than getting started content.

cescoffier · 2021-07-01T05:58:40Z

extensions/devservices/common/src/main/java/io/quarkus/devservices/common/ContainerLocator.java

+        return Arrays.stream(container.getPorts())
+                .filter(containerPort -> containerPort.getPrivatePort() != null)
+                .filter(containerPort -> containerPort.getPublicPort() != null)
+                .filter(containerPort -> containerPort.getPrivatePort() == port)


Minor: The 3 filters can be merged in a single one.

@cescoffier Yes, I did consider it, but I didn't want to make a single line too long (I prefer "vertical" over "horizontal" code) - as right now it's easier to read.
Actually now, when I saw this code again with a fresh head - I think it could be extracted to a predicate, so we will a single filter - which won't be too long at the same time.

gsmet

Once the dust has settled, please try to squash the commits a bit to have proper semantic ones if possible.

patrox · 2021-07-01T07:44:30Z

Once the dust has settled, please try to squash the commits a bit to have proper semantic ones if possible.

@gsmet Yes, this is the next step - as I believe that the dust has already settled :)

Refactor redis-client devservice to use a common container locator Refactor kafka-client devservice to use a common container locator Refactor amqp-client devservice to use a common container locator

patrox · 2021-07-01T08:37:20Z

Once the dust has settled, please try to squash the commits a bit to have proper semantic ones if possible.

@gsmet The commits were squashed and the branch was rebased to the latest ~~master~~, sorry main.

patrox · 2021-07-02T13:03:49Z

@gsmet @Ladicek I have squashed the commits so there are only two commits left.
Can you please have a look and approve if there are no more comments from your side ?

Ladicek

I think this is OK, though there's certain school of thought (relatively prevalent on the Quarkus team) that prefers anonymous classes over lambdas because of memory footprint. Since the changes from anonymous classes to lambdas in this PR are in deployment modules and hence don't affect production builds, I think that should be fine :-)

patrox · 2021-07-02T13:24:46Z

I think this is OK, though there's certain school of thought (relatively prevalent on the Quarkus team) that prefers anonymous classes over lambdas because of memory footprint. Since the changes from anonymous classes to lambdas in this PR are in deployment modules and hence don't affect production builds, I think that should be fine :-)

Thanks, @Ladicek for this insightful answer - I was actually wondering what's the rationale for such heavy usage of anonymous classes - and now I know.

commits have been squashed.

machi1990 · 2021-07-03T15:12:54Z

Thanks @patrox for the great work.

quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/documentation area/kafka area/reactive-messaging area/redis area/smallrye labels Jun 26, 2021

patrox force-pushed the common_label_based_container_locator branch from f51c039 to dc16a05 Compare June 26, 2021 20:25

patrox changed the title ~~Common label based container locator~~ Implement label-based container discovery for Redis Dev Service Jun 26, 2021

patrox mentioned this pull request Jun 26, 2021

Implement label-based container lookup for Redis Dev Service #18179

Closed

stuartwdouglas approved these changes Jun 28, 2021

View reviewed changes

machi1990 reviewed Jun 28, 2021

View reviewed changes

cescoffier requested changes Jun 28, 2021

View reviewed changes

patrox force-pushed the common_label_based_container_locator branch from 6083f73 to 1bf2b3c Compare June 28, 2021 12:40

machi1990 reviewed Jun 28, 2021

View reviewed changes

docs/src/main/asciidoc/redis.adoc Show resolved Hide resolved

machi1990 approved these changes Jun 28, 2021

View reviewed changes

machi1990 requested review from cescoffier and Ladicek June 28, 2021 18:41

Ladicek reviewed Jun 29, 2021

View reviewed changes

extensions/devservices/common/src/main/java/io/quarkus/devservices/common/ContainerAddress.java Outdated Show resolved Hide resolved

Ladicek reviewed Jun 29, 2021

View reviewed changes

docs/src/main/asciidoc/redis-dev-services.adoc Outdated Show resolved Hide resolved

cescoffier requested changes Jun 29, 2021

View reviewed changes

patrox force-pushed the common_label_based_container_locator branch 3 times, most recently from 35b32af to a95ae34 Compare June 30, 2021 12:21

stuartwdouglas reviewed Jul 1, 2021

View reviewed changes

cescoffier approved these changes Jul 1, 2021

View reviewed changes

gsmet previously requested changes Jul 1, 2021

View reviewed changes

patrox added 2 commits July 1, 2021 10:18

Implement label-based container lookup for Redis Dev Service

da56f2f

Introduce a common label-based container locator

bb67c2a

Refactor redis-client devservice to use a common container locator Refactor kafka-client devservice to use a common container locator Refactor amqp-client devservice to use a common container locator

patrox force-pushed the common_label_based_container_locator branch from a95ae34 to bb67c2a Compare July 1, 2021 08:18

machi1990 requested a review from gsmet July 1, 2021 17:49

Ladicek approved these changes Jul 2, 2021

View reviewed changes

machi1990 merged commit fdc95c6 into quarkusio:main Jul 3, 2021

quarkus-bot bot added this to the 2.1 - main milestone Jul 3, 2021

machi1990 linked an issue Jul 3, 2021 that may be closed by this pull request

Implement shared Redis Server with DevServices #18029

Closed

machi1990 mentioned this pull request Jul 3, 2021

Implement shared Redis Server with DevServices #18029

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement label-based container discovery for Redis Dev Service #18181

Implement label-based container discovery for Redis Dev Service #18181

patrox commented Jun 26, 2021 •

edited

patrox commented Jun 26, 2021

machi1990 left a comment

cescoffier Jun 28, 2021

patrox Jun 28, 2021

patrox Jun 28, 2021 •

edited

cescoffier Jun 29, 2021

patrox Jun 30, 2021

cescoffier Jun 28, 2021

quarkus-bot bot commented Jun 28, 2021 •

edited

Failing Jobs - Building `dc16a05`

patrox commented Jun 28, 2021

Ladicek commented Jun 28, 2021

machi1990 commented Jun 28, 2021

Ladicek commented Jun 28, 2021

patrox commented Jun 28, 2021

patrox commented Jun 28, 2021 •

edited

cescoffier left a comment

quarkus-bot bot commented Jun 29, 2021 •

edited

Failing Jobs - Building `a8d4b35`

patrox commented Jun 29, 2021

stuartwdouglas Jul 1, 2021

cescoffier Jul 1, 2021

cescoffier Jul 1, 2021

patrox Jul 1, 2021 •

edited

gsmet left a comment

patrox commented Jul 1, 2021

patrox commented Jul 1, 2021

patrox commented Jul 2, 2021 •

edited

Ladicek left a comment

patrox commented Jul 2, 2021

machi1990 commented Jul 3, 2021

Implement label-based container discovery for Redis Dev Service #18181

Implement label-based container discovery for Redis Dev Service #18181

Conversation

patrox commented Jun 26, 2021 • edited

patrox commented Jun 26, 2021

machi1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrox Jun 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quarkus-bot bot commented Jun 28, 2021 • edited

Failing Jobs - Building dc16a05

patrox commented Jun 28, 2021

Ladicek commented Jun 28, 2021

machi1990 commented Jun 28, 2021

Ladicek commented Jun 28, 2021

patrox commented Jun 28, 2021

patrox commented Jun 28, 2021 • edited

cescoffier left a comment

Choose a reason for hiding this comment

quarkus-bot bot commented Jun 29, 2021 • edited

Failing Jobs - Building a8d4b35

patrox commented Jun 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrox Jul 1, 2021 • edited

Choose a reason for hiding this comment

gsmet left a comment

Choose a reason for hiding this comment

patrox commented Jul 1, 2021

patrox commented Jul 1, 2021

patrox commented Jul 2, 2021 • edited

Ladicek left a comment

Choose a reason for hiding this comment

patrox commented Jul 2, 2021

machi1990 commented Jul 3, 2021

patrox commented Jun 26, 2021 •

edited

patrox Jun 28, 2021 •

edited

quarkus-bot bot commented Jun 28, 2021 •

edited

Failing Jobs - Building `dc16a05`

patrox commented Jun 28, 2021 •

edited

quarkus-bot bot commented Jun 29, 2021 •

edited

Failing Jobs - Building `a8d4b35`

patrox Jul 1, 2021 •

edited

patrox commented Jul 2, 2021 •

edited