-
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(bootstrap): add bootstrap step to clear out unknown aspect rows from the database #5148
feat(bootstrap): add bootstrap step to clear out unknown aspect rows from the database #5148
Conversation
…from the database
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.
Doing a full mysql scan on each deploy seems very dangerous. Also would take forever. Simple listUrns function took a long time (more than 10 minutes) on big instances
metadata-service/factories/src/main/java/com/linkedin/metadata/boot/BootstrapManager.java
Show resolved
Hide resolved
...rvice/factories/src/main/java/com/linkedin/metadata/boot/steps/RemoveUnknownAspectsStep.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/linkedin/datahub/upgrade/removeunknownaspects/RemoveUnknownAspectsStep.java
Outdated
Show resolved
Hide resolved
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityService.java
Outdated
Show resolved
Hide resolved
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityService.java
Show resolved
Hide resolved
metadata-io/src/main/java/com/linkedin/metadata/entity/cassandra/CassandraAspectDao.java
Outdated
Show resolved
Hide resolved
metadata-io/src/main/java/com/linkedin/metadata/entity/ebean/EbeanAspectDao.java
Show resolved
Hide resolved
public class EntityAspectIdentifier { | ||
@Nonnull String urn; | ||
@Nonnull String aspect; | ||
long version; | ||
|
||
public static EntityAspectIdentifier fromEbean(EbeanAspectV2 ebeanAspectV2) { |
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.
I really dislike the naming inconsistency of EbeanAspectV2
and CassandraAspect
:( Seems very strange that this class needs to know about specific DB implementations, also.
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.
It's a utility conversion from the lower level aspect, made more sense here to me tied to the actual higher level object it will always be used with than in a separate utility class that will never be found by others trying to do the same thing.
// Validate pre-conditions before running queries | ||
Urn entityUrn; | ||
EntitySpec entitySpec; | ||
try { | ||
entityUrn = Urn.createFromString(urn); | ||
String entityName = PegasusUtils.urnToEntityName(entityUrn); | ||
entitySpec = getEntityRegistry().getEntitySpec(entityName); | ||
Preconditions.checkState(entitySpec != null, String.format("Could not find entity definition for %s", entityName)); |
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.
Why remove these?
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.
One of them doesn't actually do anything since it will never be triggered and the other one prevents the delete from going through.
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.
Why would it prevent the delete?
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.
Don't we always populate the "aspectName" field?
Checklist