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.

Sign up for GitHub

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

Jump to bottom

Enable users to set the gRPC max message size #65

Merged
merged 2 commits into from
Apr 23, 2023
Merged

Enable users to set the gRPC max message size #65

merged 2 commits into from
Apr 23, 2023

Conversation

taeyeon-Kim
Copy link
Contributor

resolved: #64

@taeyeon-Kim taeyeon-Kim self-assigned this Apr 17, 2023
@taeyeon-Kim taeyeon-Kim requested review from a team and junoyoon as code owners April 17, 2023 15:53
@github-actions
Copy link

github-actions bot commented Apr 17, 2023 •
edited
Loading

Scavenger Test Results

155 files  155 suites   32s ⏱️
254 tests 242 ✔️ 12 💤 0 ❌
256 runs  244 ✔️ 12 💤 0 ❌

Results for commit 755bb3d.

♻️ This comment has been updated with latest results.

junoyoon
junoyoon reviewed Apr 23, 2023
View reviewed changes
doc/installation.md Outdated
@@ -36,6 +36,10 @@ You can override any configuration values with `-D` option.
To change the http(default 8080) and grpc(default 8080) ports used by Collector, you can change the settings below.
- `-Darmeria.port`: http, grpc uses 8080 by default

To change the default value of 10MB for the max message size of gRPC in Collector, change the setting below.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
To change the default value of 10MB for the max message size of gRPC in Collector, change the setting below.
To change the the max message size of gRPC in Collector, change the setting below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed it in 755bb3d

doc/installation.md Outdated
@@ -174,6 +178,10 @@ packages=com.navercorp

- Setting up the agent via `-javaagent:path/to/scavenger-agent.jar`
- Specify the config file via `-Dscavenger.configuration=path/to/scavenger.conf`
- To change the default value of 10MB for the max message size of gRPC in agent, change the setting below.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- To change the default value of 10MB for the max message size of gRPC in agent, change the setting below.
- To change the max message size of gRPC in agent, change the setting below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed it in 755bb3d

doc/installation.md Outdated
- To change the default value of 10MB for the max message size of gRPC in agent, change the setting below.
- `-Dscavenger.max-message-size`
- Supports `B`, `KB`, `MB`, `GB`, and `TB` units.
> **We recommend tuning your codeBase configuration before changing `scavenger.max-message-size`.**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
> **We recommend tuning your codeBase configuration before changing `scavenger.max-message-size`.**
> **We recommend tuning your codeBase configuration for minimizing tracing ranges before changing `scavenger.max-message-size`.**

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed it in 755bb3d

scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/publishing/GrpcClient.java Outdated
@@ -66,10 +75,11 @@ private void createNewChannelIfShutdown() {
if (channel == null || channel.isShutdown()) {
log.fine("[scavenger] creating new grpc channel");
try {
channel = createChannel();
int maxMessageSize = System.getProperty(MAX_MESSAGE_SIZE_CONFIG) != null ? maxMessageSize() : DEFAULT_MAX_MESSAGE_SIZE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it better to implement System.getProperty(MAX_MESSAGE_SIZE_CONFIG) != null ? maxMessageSize() : DEFAULT_MAX_MESSAGE_SIZE; in maxMessageSize()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed it in 755bb3d

Copy link
Collaborator

Choose a reason for hiding this comment

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

int maxMessageSize = maxMessageSize()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh... i missed it... ammend in 6722455

junoyoon
junoyoon reviewed Apr 23, 2023
View reviewed changes
doc/installation.md Outdated
@@ -36,6 +36,10 @@ You can override any configuration values with `-D` option.
To change the http(default 8080) and grpc(default 8080) ports used by Collector, you can change the settings below.
- `-Darmeria.port`: http, grpc uses 8080 by default

To change the the max message size of gRPC in Collector, change the setting below.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
To change the the max message size of gRPC in Collector, change the setting below.
To change the max message size of gRPC in Collector, change the setting below.

@taeyeon-Kim taeyeon-Kim force-pushed the feature/enable-user-set-grpc-max-message-size branch from 755bb3d to 6722455 Compare April 23, 2023 12:59
@taeyeon-Kim taeyeon-Kim force-pushed the feature/enable-user-set-grpc-max-message-size branch from 6722455 to 8c4b080 Compare April 23, 2023 13:01
@taeyeon-Kim taeyeon-Kim merged commit 5ae2e94 into develop Apr 23, 2023
@taeyeon-Kim taeyeon-Kim deleted the feature/enable-user-set-grpc-max-message-size branch April 23, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@junoyoon junoyoon junoyoon approved these changes

@kojandy kojandy kojandy approved these changes

@sohyun-ku sohyun-ku sohyun-ku approved these changes

Assignees

@taeyeon-Kim taeyeon-Kim

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable users to set the gRPC max message size
4 participants
@taeyeon-Kim @junoyoon @kojandy @sohyun-ku