-
Notifications
You must be signed in to change notification settings - Fork 113
[vpj][dvc][cc][test] Create PubSub adapter factories based on VeniceProperties #1702
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
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.
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
internal/venice-common/src/java/com/linkedin/venice/pubsub/PubSubClientsFactory.java:26
- [nitpick] Consider adding JavaDoc comments for the new generic parameters to improve code clarity and assist future maintainers.
public class PubSubClientsFactory<P extends PubSubProducerAdapter, C extends PubSubConsumerAdapter, A extends PubSubAdminAdapter> {
internal/venice-common/src/java/com/linkedin/venice/pubsub/PubSubClientsFactory.java:46
- [nitpick] Consider capturing explicit generic types in the factory method for better type inference and consistency across usages.
public static PubSubClientsFactory<? extends PubSubProducerAdapter, ? extends PubSubConsumerAdapter, ? extends PubSubAdminAdapter> fromVeniceProperties(
|
Even for draft PRs, having a clear and descriptive title is helpful. |
aa47f65 to
5c84429
Compare
...nal/venice-common/src/main/java/com/linkedin/venice/pubsub/manager/TopicMetadataFetcher.java
Show resolved
Hide resolved
clients/da-vinci-client/src/main/java/com/linkedin/davinci/DaVinciBackend.java
Outdated
Show resolved
Hide resolved
clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java
Outdated
Show resolved
Hide resolved
...nci-client/src/main/java/com/linkedin/davinci/kafka/consumer/KafkaStoreIngestionService.java
Outdated
Show resolved
Hide resolved
...nci-client/src/main/java/com/linkedin/davinci/kafka/consumer/KafkaStoreIngestionService.java
Outdated
Show resolved
Hide resolved
...nci-client/src/main/java/com/linkedin/davinci/kafka/consumer/KafkaStoreIngestionService.java
Outdated
Show resolved
Hide resolved
...s/da-vinci-client/src/main/java/com/linkedin/davinci/store/view/ChangeCaptureViewWriter.java
Outdated
Show resolved
Hide resolved
...ts/da-vinci-client/src/main/java/com/linkedin/davinci/store/view/MaterializedViewWriter.java
Outdated
Show resolved
Hide resolved
...s/venice-push-job/src/main/java/com/linkedin/venice/hadoop/input/kafka/KafkaInputFormat.java
Outdated
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/pubsub/PubSubClientsFactory.java
Outdated
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriterFactory.java
Outdated
Show resolved
Hide resolved
adf4913 to
3842960
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.
Thanks, @arjun4084346! Let’s update the PR title to use “PubSub” instead of “Kafka” for clarity and accuracy:
"[vpj][dvc][cc][test] Create PubSub adapter factories based on VeniceProperties"
internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriterFactory.java
Outdated
Show resolved
Hide resolved
...client/src/main/java/com/linkedin/davinci/consumer/VeniceChangelogConsumerClientFactory.java
Outdated
Show resolved
Hide resolved
...ice-push-job/src/main/java/com/linkedin/venice/hadoop/input/kafka/KafkaInputDictTrainer.java
Outdated
Show resolved
Hide resolved
...s/venice-push-job/src/main/java/com/linkedin/venice/hadoop/input/kafka/KafkaInputFormat.java
Outdated
Show resolved
Hide resolved
...ce-push-job/src/main/java/com/linkedin/venice/hadoop/input/kafka/KafkaInputRecordReader.java
Outdated
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/utils/DictionaryUtils.java
Outdated
Show resolved
Hide resolved
...nal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestHybrid.java
Outdated
Show resolved
Hide resolved
...java/com/linkedin/venice/integration/utils/VeniceTwoLayerMultiRegionMultiClusterWrapper.java
Outdated
Show resolved
Hide resolved
...rc/integrationTest/java/com/linkedin/venice/integration/utils/VeniceMultiClusterWrapper.java
Outdated
Show resolved
Hide resolved
...ice-push-job/src/main/java/com/linkedin/venice/hadoop/input/kafka/KafkaInputDictTrainer.java
Outdated
Show resolved
Hide resolved
...ce-push-job/src/main/java/com/linkedin/venice/hadoop/input/kafka/KafkaInputRecordReader.java
Outdated
Show resolved
Hide resolved
clients/venice-push-job/src/main/java/com/linkedin/venice/jobs/ConfigSetter.java
Outdated
Show resolved
Hide resolved
internal/venice-common/src/test/java/com/linkedin/venice/pubsub/PubSubClientsFactoryTest.java
Outdated
Show resolved
Hide resolved
...mon/src/integrationTest/java/com/linkedin/venice/integration/utils/VeniceClusterWrapper.java
Outdated
Show resolved
Hide resolved
...rc/integrationTest/java/com/linkedin/venice/integration/utils/VeniceMultiClusterWrapper.java
Outdated
Show resolved
Hide resolved
...java/com/linkedin/venice/integration/utils/VeniceTwoLayerMultiRegionMultiClusterWrapper.java
Outdated
Show resolved
Hide resolved
internal/venice-test-common/src/main/java/com/linkedin/venice/utils/TestWriteUtils.java
Outdated
Show resolved
Hide resolved
580f789 to
b807425
Compare
2a80e77 to
0801cae
Compare
clients/venice-push-job/src/test/java/com/linkedin/venice/hadoop/VenicePushJobTest.java
Outdated
Show resolved
Hide resolved
clients/venice-push-job/src/main/java/com/linkedin/venice/hadoop/VenicePushJob.java
Outdated
Show resolved
Hide resolved
...s/venice-push-job/src/main/java/com/linkedin/venice/hadoop/input/kafka/KafkaInputFormat.java
Outdated
Show resolved
Hide resolved
...ce-push-job/src/main/java/com/linkedin/venice/hadoop/input/kafka/KafkaInputRecordReader.java
Outdated
Show resolved
Hide resolved
internal/venice-client-common/src/main/java/com/linkedin/venice/utils/CollectionUtils.java
Outdated
Show resolved
Hide resolved
sushantmane
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, the changes look good. Just a few minor comments. Let’s also update the PR description and commit message to better reflect the current changes--they're a bit outdated now.
clients/venice-push-job/src/main/java/com/linkedin/venice/jobs/DataWriterComputeJob.java
Outdated
Show resolved
Hide resolved
internal/venice-client-common/src/main/java/com/linkedin/venice/utils/CollectionUtils.java
Outdated
Show resolved
Hide resolved
internal/venice-client-common/src/main/java/com/linkedin/venice/utils/CollectionUtils.java
Outdated
Show resolved
Hide resolved
sushantmane
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.
Thanks, @arjun4084346!
…roperties (linkedin#1702) passing pubsub configs to vpj and venice writer inside VPJ so admin/producer/consumer adapters of multiple types can be supported
Problem Statement
In several places, Kafka admin/consumer/admin adapter factories are created directly (
new ApacheKafkaAdminAdapterFactory()) without using venice properties. This obstructs using a different type of adapter transparently with the help of properties.We are not passing configs needed by pubsub clients to VPJ.
Solution
Code changes
Concurrency-Specific Checks
Both reviewer and PR author to verify
synchronized,RWLock) are used where needed.ConcurrentHashMap,CopyOnWriteArrayList).How was this PR tested?
Does this PR introduce any user-facing or breaking changes?