Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Nov 16, 2020

Conversation

erikjoh
Copy link
Contributor

@erikjoh erikjoh commented Nov 13, 2020

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
Copy link

linux-foundation-easycla bot commented Nov 13, 2020

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

CLA Not Signed

@erikjoh
Copy link
Contributor Author

erikjoh commented Nov 13, 2020

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

CLA Not Signed

@erikjoh
Copy link
Contributor Author

erikjoh commented Nov 13, 2020

@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
Copy link
Member

@ejona86 ejona86 left a comment

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 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
Copy link
Contributor

@sanjaypujare sanjaypujare left a comment

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

Copy link
Contributor

@sanjaypujare sanjaypujare left a comment

Choose a reason for hiding this comment

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

LG

@erikjoh
Copy link
Contributor Author

erikjoh commented Nov 16, 2020

@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
Copy link
Contributor Author

erikjoh commented Nov 16, 2020

@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
Copy link
Member

ejona86 commented Nov 16, 2020

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.

None yet

5 participants