-
Notifications
You must be signed in to change notification settings - Fork 114
[docker] Add fast client support to Docker quickstart #2466
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
base: main
Are you sure you want to change the base?
[docker] Add fast client support to Docker quickstart #2466
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds D2-based Venice fast client support to the Docker quickstart so users can query storage nodes directly (bypassing the router), and fixes server metadata URL scheme generation to respect the server’s actual SSL configuration.
Changes:
- Add D2 announcement support (router + optional server URI) for quickstart D2 service discovery.
- Introduce
D2ConfigUtils(prod utility) andFastClientQueryTool(CLI) plus build/docker packaging for avenice-client-all.jar. - Fix server-based metadata to return
http://vshttps://URLs based on server SSL enablement.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| services/venice-router/src/main/java/com/linkedin/venice/router/RouterServer.java | Adds D2 config setup + service discovery announcers during router startup |
| internal/venice-common/src/main/java/com/linkedin/venice/d2/D2ConfigUtils.java | New utility to configure D2 in ZK and create announcers |
| clients/venice-client/src/main/java/com/linkedin/venice/fastclient/FastClientQueryTool.java | New CLI for fast-client reads using D2 |
| clients/venice-client/build.gradle | Adds shadow JAR config + sets CLI main class manifest |
| services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java | Passes SSL-enabled flag to server metadata repository |
| services/venice-server/src/main/java/com/linkedin/venice/listener/ServerReadMetadataRepository.java | Uses SSL-enabled flag to generate correct routing/Helix URLs |
| internal/venice-common/src/main/java/com/linkedin/venice/utils/HelixUtils.java | Adds instanceIdToUrl(instanceId, https) overload |
| docker/venice-router/single-dc-configs/router.properties | Enables D2 announcement + adds server D2 announce config for quickstart |
| docker/venice-client/fast-client-fetch.sh | New quickstart helper script to run the fast-client CLI |
| docker/venice-client/Dockerfile | Packages the new venice-client-all.jar + helper script into the client container |
| docker/build-venice-docker-images.sh | Copies/removes the new client fat JAR into docker build context |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
services/venice-router/src/main/java/com/linkedin/venice/router/RouterServer.java
Show resolved
Hide resolved
services/venice-router/src/main/java/com/linkedin/venice/router/RouterServer.java
Outdated
Show resolved
Hide resolved
services/venice-router/src/main/java/com/linkedin/venice/router/RouterServer.java
Outdated
Show resolved
Hide resolved
clients/venice-client/src/main/java/com/linkedin/venice/fastclient/FastClientQueryTool.java
Outdated
Show resolved
Hide resolved
clients/venice-client/src/main/java/com/linkedin/venice/fastclient/FastClientQueryTool.java
Show resolved
Hide resolved
clients/venice-client/src/main/java/com/linkedin/venice/fastclient/FastClientQueryTool.java
Outdated
Show resolved
Hide resolved
e6e2ee3 to
1ef6f9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
services/venice-router/src/main/java/com/linkedin/venice/router/RouterServer.java
Show resolved
Hide resolved
clients/venice-client/src/main/java/com/linkedin/venice/fastclient/FastClientQueryTool.java
Show resolved
Hide resolved
clients/venice-client/src/main/java/com/linkedin/venice/fastclient/FastClientQueryTool.java
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/d2/D2ConfigUtils.java
Outdated
Show resolved
Hide resolved
25338e4 to
f890481
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
clients/venice-client/src/main/java/com/linkedin/venice/fastclient/FastClientQueryTool.java
Outdated
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/d2/D2ConfigUtils.java
Outdated
Show resolved
Hide resolved
services/venice-router/src/main/java/com/linkedin/venice/router/RouterServer.java
Show resolved
Hide resolved
services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java
Show resolved
Hide resolved
services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java
Show resolved
Hide resolved
...s/venice-server/src/main/java/com/linkedin/venice/listener/ServerReadMetadataRepository.java
Show resolved
Hide resolved
f890481 to
25a354b
Compare
25a354b to
6cdae17
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
services/venice-router/src/main/java/com/linkedin/venice/router/RouterServer.java
Show resolved
Hide resolved
6cdae17 to
73043ab
Compare
73043ab to
dfa2ee0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
clients/venice-client/src/main/java/com/linkedin/venice/fastclient/FastClientQueryTool.java
Show resolved
Hide resolved
services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java
Show resolved
Hide resolved
services/venice-router/src/main/java/com/linkedin/venice/router/RouterServer.java
Show resolved
Hide resolved
Add D2-based Venice fast client to the Docker quickstart, enabling direct data reads to storage servers bypassing the router. Changes: - Add D2ConfigUtils: production-ready D2 setup utility extracted from test code (setupD2Config, createD2Server, getD2Servers) - Add D2 self-announcement to RouterServer.main() for cluster discovery services, gated by router.d2.announce.enabled (default false) - Add D2 self-announcement to VeniceServer.run() for server D2 service, gated by server.d2.announce.enabled (default false) - Add FastClientQueryTool: CLI for fast-client reads using D2, with optional --insecure flag for trust-all SSL - Add shadow JAR config to venice-client build.gradle - Fix ServerReadMetadataRepository to respect server SSL config when generating instance URLs (was hardcoded to HTTPS) - Add HelixUtils.instanceIdToUrl(instanceId, https) overload - Add fast-client-fetch.sh wrapper script and Docker packaging - Update quickstart docs with fast client usage (Option B) - Add unit tests for HelixUtils, FastClientQueryTool, and ServerReadMetadataRepository SSL-aware URL generation
dfa2ee0 to
450517e
Compare
xunyin8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just some NIT.
| controller.parent.mode=false | ||
| controller.system.schema.cluster.name=venice-cluster0 | ||
| cluster.to.d2=venice-cluster0:venice-discovery | ||
| cluster.to.server.d2=venice-cluster0:venice-server-d2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include the cluster name in the corresponding server d2 too? e.g. venice-cluster0:venice-server-cluster0-d2. Otherwise it's going to be confusing when we try to run with multiple clusters.
| d2Servers.addAll(D2ConfigUtils.getD2Servers(zkAddress, d2ClusterName, localUri)); | ||
| } | ||
|
|
||
| // Announce the global discovery service only if not already covered by cluster.to.d2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing, why would cluster.to.d2 ever have DEFAULT_CLUSTER_DISCOVERY_D2_SERVICE_NAME as a value? The intention of cluster.to.d2 is a mapping of venice cluster to corresponding d2 service/cluster name.
Summary
Adds Venice fast client (D2-based) support to the Docker quickstart, which directly reads data from storage servers bypassing the router. This addresses issue #2150 (D2Server integration).
router.d2.announce.enabledproperty (default false)D2TestUtilsfor setting up D2 config and server announcersjava -jar venice-client-all.jar <store> <key> <zk_address>)ServerReadMetadataRepositorynow uses the server's actual SSL configuration for metadata response URLs instead of hardcoding HTTPS. AddedHelixUtils.instanceIdToUrl(instanceId, https)overloadfast-client-fetch.shscript, shadow JAR build for venice-client, and updated Dockerfile/build scriptTest plan
HelixUtilsTest,ServerReadMetadataRepositoryTest(3/3 tests)fetch.sh)1→val1,25→val25,50→val50,100→val100999→nullCloses #2150