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

Feature/oss/update to v2 endpoints #4128

Merged

Conversation

RyanHolstien
Copy link
Collaborator

Updates all internal usages of old Snapshot endpoints to V2 endpoints for Java classes

@github-actions
Copy link

github-actions bot commented Feb 11, 2022

Unit Test Results (build & test)

  70 files  ±0    70 suites  ±0   10m 46s ⏱️ - 2m 9s
609 tests ±0  550 ✔️ ±0  59 💤 ±0  0 ±0 

Results for commit 288937d. ± Comparison against base commit 06bb033.

♻️ This comment has been updated with latest results.

Constants.ASPECT_LATEST_VERSION,
context.getAuthentication());
return keyAspect != null;
Urn userUrn = Urn.createFromString(userUrnStr);
Copy link
Collaborator

Choose a reason for hiding this comment

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

So I think previously if there was no key aspect, this would getAspectOrNull would return null... But I believe that batchGetV2 artificially injects a key aspect into the result set to maintain our previous behavior. In this particular case it won't be a huge deal but it might be in other cases... Gabe added an "exists" API in GMS that we should migrate this to instead...

Don't may not need to address in this PR, but definitely want to call it out

_datasetHydrator.hydrateFromEntityResponse(document, entityResponse);
break;
default:
log.error("Unable to find valid hydrator for entity type: {} urn: {}", entityResponse.getEntityName(), urn);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome

Copy link
Collaborator

@jjoyce0510 jjoyce0510 left a comment

Choose a reason for hiding this comment

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

LGTM. Amazing PR, thank you @RyanHolstien !

@shirshanka shirshanka merged commit a251d58 into datahub-project:master Feb 15, 2022
hevandro-veiga pushed a commit to hevandro-veiga/datahub that referenced this pull request Feb 18, 2022
maggiehays pushed a commit to maggiehays/datahub that referenced this pull request Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants