-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(mae/mcl): Make ingestAspect produce both MCLs and MAEs #3737
feat(mae/mcl): Make ingestAspect produce both MCLs and MAEs #3737
Conversation
produceMAETimer.stop(); | ||
|
||
Timer.Context produceMCLTimer = MetricUtils.timer(this.getClass(), "produceMCL").time(); | ||
produceMetadataChangeLog(urn, entityName, aspectName, aspectSpec, oldValue, updatedValue, oldSystemMetadata, |
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.
Can we produce to BOTH? (And deprecate consumption via MAE processor?)
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.
Added produce to both and removed MAE consumer
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.
Note the produce calls are async under the hood so no impact on latency
|
||
_producer.produceMetadataAuditEvent(urn, oldSnapshot, newSnapshot, oldSystemMetadata, newSystemMetadata, operation); | ||
protected Snapshot buildKeySnapshot(@Nonnull final Urn urn) { |
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.
Nit: this should prob go below produceMetadataAuditEventForKey
@@ -143,8 +147,17 @@ public void testIngestGetEntity() throws Exception { | |||
assertTrue(DataTemplateUtil.areEqual(expectedKey, | |||
readEntity.getValue().getCorpUserSnapshot().getAspects().get(0).getCorpUserKey())); // Key + Info aspect. | |||
|
|||
ArgumentCaptor<MetadataChangeLog> mclCaptor = ArgumentCaptor.forClass(MetadataChangeLog.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.
nice!!
@@ -9,7 +9,7 @@ | |||
public class MaeConsumerApplication { | |||
|
|||
public static void main(String[] args) { | |||
Class<?>[] primarySources = {MaeConsumerApplication.class, com.linkedin.metadata.kafka.MaeConsumerConfig.class}; | |||
Class<?>[] primarySources = {MaeConsumerApplication.class, MclConsumerConfig.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.
Yess!
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.
LGTM
We are migrating away from the legacy MCE/MAE setup to MCP/MCL setup detailed in this doc https://datahubproject.io/docs/advanced/mcp-mcl/
As the first step, we will change ingestAspect to produce MCLs and MAEs, and remove MAE processor to consume only from MCLs.
We are leaving MAEs as is for now, since there are external dependencies outside of DataHub that may be consuming MAEs. Note, the recommendation is to move to MCLs.
Tested locally and made sure MCLs are created correctly and that MAEs are still correctly produced.
Checklist