Skip to content

xds: add support for setting bootstrap file with java system property - #7620

Merged
ejona86 merged 1 commit into
grpc:masterfrom
erikjoh:xds-bootstrap-property
Nov 16, 2020
Merged

xds: add support for setting bootstrap file with java system property#7620
ejona86 merged 1 commit into
grpc:masterfrom
erikjoh:xds-bootstrap-property

Conversation

@erikjoh

@erikjoh erikjoh commented Nov 13, 2020

Copy link
Copy Markdown
Contributor

While most languages support setting environment variables during runtime,
Java does not. In Java, the preferred approach is to use Java System Properties
in order so specify configuration options. By checking for the existence
of the io.grpc.xds.bootstrap property if GRPC_XDS_BOOTSTRAP is not found,
it is possible to either supply the bootstrap location during runtime or as
a Java argument. The environment variable still takes precedence in order
to not break any existing documentation.

For discussion see #7605 .

@linux-foundation-easycla

linux-foundation-easycla Bot commented Nov 13, 2020

Copy link
Copy Markdown

CLA Signed

The committers are authorized under a signed CLA.

@erikjoh erikjoh closed this Nov 13, 2020
@erikjoh erikjoh reopened this Nov 13, 2020
@linux-foundation-easycla

Copy link
Copy Markdown

CLA Not Signed

@erikjoh

erikjoh commented Nov 13, 2020

Copy link
Copy Markdown
Contributor Author

Blocked waiting for Corporate CLA approval.

@erikjoh erikjoh closed this Nov 13, 2020
@erikjoh erikjoh reopened this Nov 13, 2020
@linux-foundation-easycla

Copy link
Copy Markdown

CLA Not Signed

@erikjoh

erikjoh commented Nov 13, 2020

Copy link
Copy Markdown
Contributor Author

@ejona86 @sanjaypujare FYI the link here https://github.com/grpc/grpc-java/blob/master/CONTRIBUTING.md#legal-requirements (https://identity.linuxfoundation.org/projects/cncf) goes to the old and now outdated system.
We finally figured out how to get me signed though so now this PR is ready for review 🙂

@erikjoh erikjoh closed this Nov 13, 2020
@erikjoh erikjoh reopened this Nov 13, 2020

@ejona86 ejona86 left a 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.

It seems like maybe we don't need to mention the system property in the example, but we don't actually have another place to put that documentation so it seems best for now.

@ejona86
ejona86 requested a review from sanjaypujare November 13, 2020 23:45
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 13, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 13, 2020
Comment thread xds/src/main/java/io/grpc/xds/Bootstrapper.java
Comment thread examples/example-xds/README.md Outdated

@sanjaypujare sanjaypujare 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.

Added 2 comments but looks good.

While most languages support setting environment variables during runtime,
Java does not. In Java, the preferred approach is to use Java System Properties
in order so specify configuration options. By checking for the existence
of the io.grpc.xds.bootstrap property if GRPC_XDS_BOOTSTRAP is not found,
it is possible to either supply the bootstrap location during runtime or as
a Java argument. The environment variable still takes precedence in order
to not break any existing documentation.
@erikjoh
erikjoh force-pushed the xds-bootstrap-property branch from f5065b5 to 6ff16a0 Compare November 16, 2020 16:44
@sanjaypujare

Copy link
Copy Markdown
Contributor

@erikjoh it is better if you don't squash commits and "force-push" them - that way it is easier to see the changes made in the second commit. While merging the commits can be squashed.

@sanjaypujare sanjaypujare 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.

LG

@erikjoh

erikjoh commented Nov 16, 2020

Copy link
Copy Markdown
Contributor Author

@erikjoh it is better if you don't squash commits and "force-push" them - that way it is easier to see the changes made in the second commit. While merging the commits can be squashed.

Ah sorry, I don't use the squash merge feature a lot! Noted for the next time 🙂
You'll have to merge it once the build finishes since I don't have permission to do so.
Thanks for taking the time to have a look at this!

@erikjoh

erikjoh commented Nov 16, 2020

Copy link
Copy Markdown
Contributor Author

@sanjaypujare That build failure seems unrelated to this since the class was introduced recently and does not exist on my branch even. Doesn't seem like I have any option to rerun the build.

io.grpc.xds.SharedCallCounterMapTest > autoCleanUp FAILED
    expected to be empty
    but was: {cluster-foo.googleapis.com={null=io.grpc.xds.SharedCallCounterMap$CounterReference@21c8125f}}
        at io.grpc.xds.SharedCallCounterMapTest.autoCleanUp(SharedCallCounterMapTest.java:57)

@voidzcy voidzcy added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Nov 16, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Nov 16, 2020
@voidzcy voidzcy closed this Nov 16, 2020
@voidzcy voidzcy reopened this Nov 16, 2020
@ejona86
ejona86 merged commit 19923df into grpc:master Nov 16, 2020
@ejona86

ejona86 commented Nov 16, 2020

Copy link
Copy Markdown
Member

Thank you!

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jun 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants