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.

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

lint(test): remove unused imports, other test fixes #7659

Merged

Conversation

david-leifker
Copy link
Collaborator

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

@vercel
Copy link

vercel bot commented Mar 21, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
docs-website ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 21, 2023 at 8:47PM (UTC)

@david-leifker david-leifker changed the title Update AspectGenerationUtils.java lint(test): remove unused imports Mar 21, 2023
@github-actions github-actions bot added the product PR or Issue related to the DataHub UI/UX label Mar 21, 2023
@david-leifker david-leifker changed the title lint(test): remove unused imports lint(test): remove unused imports, other test fixes Mar 21, 2023
@@ -1084,7 +1084,7 @@ public RestoreIndicesResult restoreIndices(@Nonnull RestoreIndicesArgs args, @No
logger.accept(String.format(
"Reading rows %s through %s from the aspects table completed.", args.start, args.start + args.batchSize));

for (EbeanAspectV2 aspect : rows.getList()) {
for (EbeanAspectV2 aspect : rows != null ? rows.getList() : List.<EbeanAspectV2>of()) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rows is null when cassandra

Copy link
Collaborator

Choose a reason for hiding this comment

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

Really? That's wild

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cassandra will return null since it doesn't implement the paged method. Not new, but there seems to be a new test.

@@ -107,6 +107,7 @@ public boolean setRetention(@Nullable String entityName, @Nullable String aspect
GenericAspect retentionAspect = GenericRecordUtils.serializeAspect(retentionConfig);
aspectProposal.setAspect(retentionAspect);
aspectProposal.setAspectName(Constants.DATAHUB_RETENTION_ASPECT);
aspectProposal.setChangeType(ChangeType.UPSERT);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

seems like we just need to set this

Copy link
Collaborator

Choose a reason for hiding this comment

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

was something breaking due to not having this? why didn't we see before

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This breaks a test, seems like if the change type is null it will fail a validation check. If non-timeseries, then this must be either PATCH or UPSERT. Null is no longer valid.

assertNull(mcl.getPreviousSystemMetadata());
assertEquals(mcl.getChangeType(), ChangeType.RESTATE);
assertEquals(mcl.getSystemMetadata().getProperties().get(FORCE_INDEXING_KEY), "true");
if (this instanceof EbeanEntityServiceTest) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

only valid for ebean, not cassandra

@@ -1084,7 +1084,7 @@ public RestoreIndicesResult restoreIndices(@Nonnull RestoreIndicesArgs args, @No
logger.accept(String.format(
"Reading rows %s through %s from the aspects table completed.", args.start, args.start + args.batchSize));

for (EbeanAspectV2 aspect : rows.getList()) {
for (EbeanAspectV2 aspect : rows != null ? rows.getList() : List.<EbeanAspectV2>of()) {

Check warning

Code scanning / QDJVMC

Constant values

Condition 'rows != null' is always 'true'
@@ -1084,7 +1084,7 @@
logger.accept(String.format(
"Reading rows %s through %s from the aspects table completed.", args.start, args.start + args.batchSize));

for (EbeanAspectV2 aspect : rows.getList()) {
for (EbeanAspectV2 aspect : rows != null ? rows.getList() : List.<EbeanAspectV2>of()) {

Check warning

Code scanning / QDJVMC

Constant values

Condition 'rows != null' is always 'true'
@david-leifker david-leifker merged commit 1b15dd4 into datahub-project:master Mar 21, 2023
shirshanka pushed a commit to shirshanka/datahub that referenced this pull request Mar 22, 2023
shirshanka pushed a commit to shirshanka/datahub that referenced this pull request Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants