Skip to content

Grpc#3733

Merged
elharo merged 5 commits into
masterfrom
grpc
Oct 16, 2018
Merged

Grpc#3733
elharo merged 5 commits into
masterfrom
grpc

Conversation

@elharo

@elharo elharo commented Sep 26, 2018

Copy link
Copy Markdown
Contributor

It's not immediately obvious why these two pom.xml files separately declare versions.

@elharo elharo requested a review from pongad as a code owner September 26, 2018 18:33
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 26, 2018

@pongad pongad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wait sorry. I think we need to update GAX's gRPC as well right...?

@elharo

elharo commented Sep 26, 2018

Copy link
Copy Markdown
Contributor Author

Possibly. This is an experiment to see if it breaks. Not quite ready to commit to this yet. One thing I'm curious about is why we're setting these versions in multiple places.

@pongad

pongad commented Sep 26, 2018

Copy link
Copy Markdown
Contributor

@elharo I think it's because the api-grpc and cloud-clients don't share a parent. This might just be someting carried over from when they were in different repositories. @vam-google Can you comment?

@elharo

elharo commented Sep 26, 2018

Copy link
Copy Markdown
Contributor Author

Yes, that seems right. Should they share a parent? or perhaps each import a BOM that manages the dependencies for both?

@elharo

elharo commented Sep 26, 2018

Copy link
Copy Markdown
Contributor Author

Theoretically we shouldn't need to upgrade GAX at the same time since it's on 1.13.1 and 1.15.0 (minor version bump) is supposed to be backwards compatible with that. But I wouldn't bet heavily on it.

@elharo elharo requested a review from a team October 15, 2018 22:10
@vam-google

vam-google commented Oct 15, 2018

Copy link
Copy Markdown
Contributor

@elharo, @pongad google-api-grpc and google-cloud-clients have different group ids (com.google.api.grpc and com.google.cloud respectively). Putting those under same parent is against maven's naming conventions (i'm not even sure that it will compile/publish, or at least not output severe warnings in the console on build). This separation is historical (happened much earlier than grpc packages migration to google-cloud-java). It partially makes sense (since all the grpc packages are not necessarily about cloud). To adhere to maven naming convention the parent must be in group com.google but that is too generic (like defining a parent pom for the whole google under google-cloud-java).

So the current separation is by design. It is annoying to have to repeat same dependency multiple times, but unfortunately it is not the only place (there are many dependencies duplications, not only in this repo), so fixing it here will not fix the actual problem. How to fix that dependency duplication problem is actually a very complicated problem (out of scope here).

(one of the first googled stackoverflow links regarding parent-child groupId relation)

@elharo

elharo commented Oct 15, 2018

Copy link
Copy Markdown
Contributor Author

@vam-google Do you have a reference for not putting different group IDs under the same parent? Happy not to do it here, but it may be relevant for some PRs in other repos I'm looking at.

@vam-google

Copy link
Copy Markdown
Contributor

When I was doing it I remember that putting those under same parent didn't really work (I'm not saying it did not compile, but it caused more problems than solved). As for the reference, the one, with pretty weak wording (describing it only as "A good way", not a "must") is in the maven documentation:

"A good way to determine the granularity of the groupId is to use the project structure. That is, if the current project is a multiple module project, it should append a new identifier to the parent's groupId. For example, org.apache.maven, org.apache.maven.plugins, org.apache.maven.reporting.

Notice, that children must share prefix, the only way for our case to share prefix is to have com.google group as a parent, which is too generic. Aslo there were other reasons to keep those separate (having common parent solves some problems, but also makes the whole project structure more monolithic).

@elharo

elharo commented Oct 16, 2018

Copy link
Copy Markdown
Contributor Author

Looks like the CI hung. Anyone happen to know how to force it to restart?

@garrettjonesgoogle

Copy link
Copy Markdown
Contributor

@chingor13 how do we trigger Kokoro to run?

@chingor13

chingor13 commented Oct 16, 2018

Copy link
Copy Markdown
Contributor

We add the kokoro:force-run label and Kokoro will pick it up.

@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 16, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 16, 2018

@chingor13 chingor13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, @garrettjonesgoogle can merge when gax is updated as well.

@garrettjonesgoogle

Copy link
Copy Markdown
Contributor

It's fine to merge this before GAX

@elharo elharo merged commit a45fe30 into master Oct 16, 2018
@elharo elharo deleted the grpc branch October 16, 2018 21:32
@JesseLovelace JesseLovelace mentioned this pull request Oct 25, 2018
suztomo pushed a commit that referenced this pull request Mar 9, 2026
lqiu96 pushed a commit that referenced this pull request Mar 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants