Skip to content

grpc-benchmark: add generated code and TLS certificates#2699

Open
arjan-bal wants to merge 3 commits into
grpc:masterfrom
arjan-bal:grpc-benchmarks
Open

grpc-benchmark: add generated code and TLS certificates#2699
arjan-bal wants to merge 3 commits into
grpc:masterfrom
arjan-bal:grpc-benchmarks

Conversation

@arjan-bal

@arjan-bal arjan-bal commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Addresses: #2666

This PR add the proto files and TLS certificates required to run the gRPC benchmarks.

The proto files are copied from the grpc-proto repository.

The TLS certs are copied from the gRPC Go repo which seem to be copied from the core repo.

@arjan-bal arjan-bal changed the title benchmark: add generated code and TLS certificates grpc-benchmark: add generated code and TLS certificates Jun 22, 2026
@arjan-bal arjan-bal requested review from dfawley and ejona86 June 22, 2026 18:48

@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 files are missing; I don't see the actual benchmark workers.

Comment thread grpc-benchmark/Cargo.toml Outdated
Comment thread grpc-benchmark/proto/grpc/testing/benchmark_service.proto Outdated
Comment thread grpc-benchmark/build.rs Outdated
@arjan-bal arjan-bal assigned arjan-bal and unassigned ejona86 and dfawley Jun 23, 2026
@arjan-bal

Copy link
Copy Markdown
Contributor Author

It seems like files are missing; I don't see the actual benchmark workers.

Now I've copied all the files over from https://github.com/grpc/grpc-proto/blob/master/grpc/testing, except test.proto which seems to be used for xDS tests. The worker service is defined in grpc-benchmark/proto/grpc/testing/worker_service.proto.

@arjan-bal arjan-bal assigned ejona86 and unassigned arjan-bal Jun 23, 2026
@arjan-bal arjan-bal requested a review from ejona86 June 23, 2026 06:20
@arjan-bal arjan-bal assigned arjan-bal and unassigned ejona86 Jun 23, 2026
@arjan-bal

Copy link
Copy Markdown
Contributor Author

While working on the worker implementation, I realized that we need a worker server (not a client), so I switched to generating the worker service code for Tonic.

@arjan-bal arjan-bal assigned ejona86 and unassigned arjan-bal Jun 23, 2026

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

I still don't understand this PR. How does this fix #2666? I don't see any client/server code that implements any functionality.

@arjan-bal

Copy link
Copy Markdown
Contributor Author

This is the first in a series of PRs addressing #2666. I intentionally used "Addresses" instead of "Fixes" so GitHub links the PR without automatically closing the issue.

I originally drafted the benchmarking binary using tonic clients and servers last year (see arjan-bal#1). To make the code easier to review, I'm splitting the changes into self-contained parts. The next PR in the chain, which contains the worker and benchmark server implementation, is #2706.

@arjan-bal arjan-bal requested a review from ejona86 June 26, 2026 08:48
@ejona86

ejona86 commented Jun 26, 2026

Copy link
Copy Markdown
Member

But "addresses" means the same as "fixes" in this context, as it implies completion. So this specific PR is not addressing the issue. I typically see language like "contributes to" or "as part of addressing" for this sort of thing. (The README was similarly misleading in that it said it contained the implementations.)

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

Fix the "addresses" language to make clear this is only the first piece.

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.

What is server.key? Should this be server1.key? I don't see how someone would know that these are specific shared files, especially if they are renamed. We need some documentation saying this is a copy from grpc/src/core/tsi .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants