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

fix(ui): Fix download to CSV flow using Scroll across entities api #7629

Merged
merged 7 commits into from
Mar 21, 2023

Conversation

jjoyce0510
Copy link
Collaborator

@jjoyce0510 jjoyce0510 commented Mar 18, 2023

Summary

In this PR we move to using scrollAcrossEntities api which allows for deep pagination against elasticsearch. This fixes download as csv which did not previously work for large data sets (greater than a few thousand)

Also, making scroll correctly apply the selected View.

Validation

Tested against 10k entities locally.

Status

Ready for review

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 the product PR or Issue related to the DataHub UI/UX label Mar 18, 2023
Copy link
Contributor

@aditya-radhakrishnan aditya-radhakrishnan left a comment

Choose a reason for hiding this comment

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

Generally looks good, have some small comments. Think we should use environment variables when possible to allow customization when deploying.

Can we also add some Cypress tests to confirm this is working properly for large numbers of search results?

datahub-web-react/src/graphql/scroll.graphql Show resolved Hide resolved
@@ -94,3 +94,5 @@ export enum UnionType {
AND,
OR,
}

export const SCROLL_KEEP_ALIVE_TIME = '10m'; // TODO: Determine if this is sufficient.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this an environment variable instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Environment variable? That means extending a config endpoint right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Kinda blows up scope to adding this to our appConfig GraphQL API. Let me confirm with Ryan on platform team whether this is something we'd need to allow config on

@jjoyce0510
Copy link
Collaborator Author

Adding Cypress Tests for large downloads is kinda a scary proposition because it can take a long time to get the requisite amount of data into the instance. Took me ~30 minutes to get ~7k entities into my own local instance. I'd hesitate to do that. Maybe a better solve is to have a download as csv cypress test generally?

@jjoyce0510
Copy link
Collaborator Author

Just chatted with Ryan - he mentioned that we can omit the parameter altogether for now! Going to remove it...

@vercel
Copy link

vercel bot commented Mar 20, 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 4:08AM (UTC)

@vercel vercel bot temporarily deployed to Preview March 20, 2023 21:15 Inactive
@vercel vercel bot temporarily deployed to Preview March 21, 2023 04:08 Inactive
@jjoyce0510 jjoyce0510 merged commit 9ec86a5 into datahub-project:master Mar 21, 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

3 participants