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

[front] Shadow read content nodes from core and compute diff #10061

Merged
merged 7 commits into from
Jan 17, 2025

Conversation

aubin-tchoi
Copy link
Contributor

Description

  • Closes #1937
  • Shadow reads the content nodes from core, compute and log the diff with the content nodes in connectors.

Risk

  • low since the nodes are not used.

Deploy Plan

  • Deploy front.

Copy link
Contributor

@philipperolet philipperolet left a comment

Choose a reason for hiding this comment

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

Thinking about it, the endstate will be a single call, not differentiating whether the datasource is static or managed right? so I think the best would be to shadow read at the toplevel function; wdyt?


// shadow read from core
const coreContentNodesRes =
await getContentNodesForManagedDataSourceViewFromCore(
Copy link
Contributor

Choose a reason for hiding this comment

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

what about turning that call into getContentNodesForDatasourceViewCore doing both static & managed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes indeed, next step would have been to move anyway!

…odesForDataSourceViewFromCore and handle static data sources altogether
Copy link
Contributor

@philipperolet philipperolet left a comment

Choose a reason for hiding this comment

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

Great 👍 Left a suggestion

"[CoreNodes] Could not fetch content nodes from core"
);
} else if (coreContentNodesRes.isOk()) {
if (coreContentNodesRes.value.total !== contentNodesResult.total) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: core defaults to 100 results while connectors is not paginated (returns everything)
I suggest switching to 250 in searchNodes from the core call for now to limit the logs on count mismatch that are just related to that (since it's not actually something to fix)

@aubin-tchoi
Copy link
Contributor Author

We are probably going to get a lot of false positives, notably on lastUpdatedAt, will keep everything for now and trim on the way.

@aubin-tchoi aubin-tchoi merged commit 7df36dd into main Jan 17, 2025
3 checks passed
@aubin-tchoi aubin-tchoi deleted the shadow-read branch January 17, 2025 16:09
tdraier pushed a commit that referenced this pull request Jan 20, 2025
* add shadow read and content nodes comparison

* add a log of extra core nodes

* fix the diff for arrays (parents)

* pass the parentsIn when no internalIds are passed

* turn getContentNodesForManagedDataSourceViewFromCore into getContentNodesForDataSourceViewFromCore and handle static data sources altogether

* 📝

* bump the limit to 250 on searchNodse
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.

2 participants