Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(ebean): refactor, fix & optimize #12613

Merged
merged 1 commit into from
Feb 19, 2025
Merged

fix(ebean): refactor, fix & optimize #12613

merged 1 commit into from
Feb 19, 2025

Conversation

david-leifker
Copy link
Collaborator

@david-leifker david-leifker commented Feb 12, 2025 •
edited
Loading

  • add rollback on pure no-op transactions
  • add test coverage for sql
  • ensure optimized sql statements
  • refactor for easier testing
  • update version correction handling

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions github-actions bot added product PR or Issue related to the DataHub UI/UX devops PR or Issue related to DataHub backend & deployment labels Feb 12, 2025
@datahub-cyborg datahub-cyborg bot added the needs-review Label for PRs that need review from a maintainer. label Feb 12, 2025
@codecov Codecov
Copy link

codecov bot commented Feb 12, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 80.59701% with 78 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...ava/com/linkedin/metadata/aspect/EntityAspect.java 62.50% 15 Missing and 3 partials ⚠️
...kedin/metadata/entity/ebean/EbeanSystemAspect.java 79.54% 15 Missing and 3 partials ⚠️
...om/linkedin/metadata/entity/EntityServiceImpl.java 89.91% 7 Missing and 5 partials ⚠️
...linkedin/metadata/entity/ebean/EbeanAspectDao.java 80.76% 6 Missing and 4 partials ⚠️
...m/linkedin/metadata/aspect/batch/AspectsBatch.java 0.00% 6 Missing ⚠️
...inkedin/metadata/aspect/EnvelopedSystemAspect.java 81.81% 2 Missing ⚠️
...va/com/linkedin/metadata/aspect/batch/MCPItem.java 0.00% 2 Missing ⚠️
...m/linkedin/metadata/utils/SystemMetadataUtils.java 33.33% 1 Missing and 1 partial ⚠️
.../metadata/entity/cassandra/CassandraAspectDao.java 90.00% 0 Missing and 2 partials ⚠️
...ahub/upgrade/restorebackup/RestoreStorageStep.java 0.00% 1 Missing ⚠️
... and 5 more

❌ Your patch status has failed because the patch coverage (56.96%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Files with missing lines Coverage Δ
...upgrade/system/bootstrapmcps/BootstrapMCPStep.java 73.33% <100.00%> (ø)
...upgrade/system/bootstrapmcps/BootstrapMCPUtil.java 74.19% <100.00%> (+0.28%) ⬆️
...hub/upgrade/system/elasticsearch/BuildIndices.java 42.50% <ø> (ø)
...in/java/com/linkedin/metadata/aspect/ReadItem.java 33.33% <ø> (ø)
.../com/linkedin/metadata/aspect/batch/ChangeMCP.java 15.00% <ø> (ø)
...va/com/linkedin/metadata/aspect/batch/MCLItem.java 0.00% <ø> (ø)
...nkedin/metadata/entity/EntityAspectIdentifier.java 55.55% <100.00%> (+5.55%) ⬆️
...din/metadata/entity/cassandra/CassandraAspect.java 76.47% <ø> (ø)
...ta/entity/cassandra/CassandraRetentionService.java 69.01% <ø> (ø)
.../linkedin/metadata/entity/ebean/EbeanAspectV2.java 100.00% <100.00%> (ø)
... and 30 more

... and 4 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d762f0...6f60e0c. Read the comment docs.

@david-leifker david-leifker force-pushed the batch-ebean-orm-preservation branch from 28e4fec to cdc1275 Compare February 12, 2025 23:03
@david-leifker david-leifker force-pushed the batch-ebean-orm-preservation branch 2 times, most recently from cdc1275 to 4fa0735 Compare February 13, 2025 03:56
@david-leifker david-leifker force-pushed the batch-ebean-orm-preservation branch from 4fa0735 to 952131d Compare February 13, 2025 16:30
@david-leifker david-leifker force-pushed the batch-ebean-orm-preservation branch from 952131d to 026fc03 Compare February 13, 2025 17:34
@david-leifker david-leifker force-pushed the batch-ebean-orm-preservation branch from 026fc03 to 3c00efc Compare February 14, 2025 01:51
@david-leifker david-leifker force-pushed the batch-ebean-orm-preservation branch from 3c00efc to 01263a9 Compare February 14, 2025 16:40
@david-leifker david-leifker force-pushed the batch-ebean-orm-preservation branch from 01263a9 to 9c9aed4 Compare February 17, 2025 05:12
@david-leifker david-leifker force-pushed the batch-ebean-orm-preservation branch from 9c9aed4 to 5016906 Compare February 17, 2025 14:52
@david-leifker david-leifker force-pushed the batch-ebean-orm-preservation branch from 5016906 to 925ec8f Compare February 17, 2025 19:53
@david-leifker david-leifker mentioned this pull request Feb 18, 2025
Open
@david-leifker david-leifker changed the title refactor(ebean): refactor & optimize refactor(ebean): refactor, fix & optimize Feb 19, 2025
@david-leifker david-leifker changed the title refactor(ebean): refactor, fix & optimize fix(ebean): refactor, fix & optimize Feb 19, 2025
@david-leifker
refactor(ebean): refactor & optimize
6f60e0c 
* add test coverage for sql
* ensure optimized sql statements
* refactor for easier testing
* update version correction handling
@david-leifker david-leifker force-pushed the batch-ebean-orm-preservation branch from 925ec8f to 6f60e0c Compare February 19, 2025 14:51
@mitchdawson1982 mitchdawson1982 mentioned this pull request Feb 19, 2025
Open
RyanHolstien
RyanHolstien approved these changes Feb 19, 2025
View reviewed changes
entity-registry/src/main/java/com/linkedin/metadata/aspect/EntityAspect.java
@Setter @Nullable private RecordTemplate recordTemplate;

@Setter @Nullable private SystemMetadata systemMetadata;
@Setter @Nullable private AuditStamp auditStamp;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Might be useful to specify in the naming that this is the createdAuditstamp

entity-registry/src/main/java/com/linkedin/metadata/aspect/EntityAspect.java
public EntityAspect.EntitySystemAspect forUpdate(
@Nonnull EntityAspect entityAspect, @Nonnull EntityRegistry entityRegistry) {

this.entityAspect = entityAspect;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it will make sense once I see how this is used, but it's confusing at this point why we only set the EntitySystemAspect.entityAspect in update scenarios when we have a non-null one in both.

...o/metadata-io-api/src/main/java/com/linkedin/metadata/entity/ebean/batch/ChangeItemImpl.java
private final Map<String, String> headers;

@Override
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, okay was wondering why the + 1 was needed now where the setter was previously getting called.

@datahub-cyborg datahub-cyborg bot added pending-submitter-merge and removed needs-review Label for PRs that need review from a maintainer. labels Feb 19, 2025
@david-leifker david-leifker merged commit d180544 into datahub-project:master Feb 19, 2025
191 of 197 checks passed
shirshanka pushed a commit to shirshanka/datahub that referenced this pull request Mar 3, 2025
@david-leifker @shirshanka
fix(ebean): refactor, fix & optimize (datahub-project#12613)
28d045f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RyanHolstien RyanHolstien RyanHolstien approved these changes

Assignees
No one assigned
Labels
devops PR or Issue related to DataHub backend & deployment pending-submitter-merge product PR or Issue related to the DataHub UI/UX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@david-leifker @RyanHolstien