-
Notifications
You must be signed in to change notification settings - Fork 58
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
Added RPC invocation for PreUpdate #443
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #443 +/- ##
============================================
+ Coverage 67.63% 67.72% +0.09%
- Complexity 1548 1576 +28
============================================
Files 138 142 +4
Lines 6077 6129 +52
Branches 658 661 +3
============================================
+ Hits 4110 4151 +41
- Misses 1688 1699 +11
Partials 279 279 ☔ View full report in Codecov by Sentry. |
dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java
Outdated
Show resolved
Hide resolved
dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java
Outdated
Show resolved
Hide resolved
dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java
Outdated
Show resolved
Hide resolved
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.
could you help understand why we’re not appending on the solution we built for DatasetAccountableOwnership? this seems to be a new framework
dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java
Outdated
Show resolved
Hide resolved
dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java
Outdated
Show resolved
Hide resolved
dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java
Outdated
Show resolved
Hide resolved
dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java
Outdated
Show resolved
Hide resolved
/** | ||
* Set pre ingestion aspect registry for grpc implementation. | ||
*/ | ||
public void setGrpcPreUpdateRegistry(@Nullable GrpcPreUpdateRegistry grpcPreUpdateRegistry) { |
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.
Where do we call this method? Should we do it through inject?
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.
Yes, we will inject it in the AssetClass where we call the ingestion method.
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.
For RestliRegistry, I was setting it in DatasetLocalDao.
* @param aspectClass aspect class | ||
* @param preUpdateRoutingMap pre update routing map | ||
*/ | ||
public void registerPreUpdateLambda(@Nonnull Class<? extends RecordTemplate> aspectClass, |
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.
again, who should register this lambda?
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.
We can create a Factory PreUpdateAspectRegistryFactory
PreUpdateAspectRegistryFactory extends SimpleSingletonFactory<GrpcPreUpdateRegistry> {
protected GrpcPreUpdateRegistry createInstance(ConfigView view) {
return GrpcPreUpdateRegistry.registerPreUpdateLambda(DatasetAccountableOwnership.class, getBean(DatasetAccountableOwnershipPreUpdateClient
.class));
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.
Let's include all those detail in your doc and trying to see which part can be covered by SMO in the future
Summary
Added grpcPreUpdateRouting method to BaseLocalDao. We will inject routingMap in BaseLocalDao through the setter method and then get the BlockingStub, RequestBuilder, and MethodDescriptor from the routingMap and invoke the RPC method.
Testing Done
./gradlew build
Checklist