[connectors] feature: Update fullsync with signals to avoid complete resync #7494

tdraier · 2024-09-18T12:41:29Z

fixes: #6368

Description

Starts fullSync with signals allowing to add or remove nodes from the current sync.
Based on #6711

Risk

may break microsoft fullsync, can be rollbacked

Deploy Plan

deploy connectors

philipperolet

👋 not sure I get the use of signals here, let's talk irl

philipperolet · 2024-09-18T13:29:36Z

connectors/src/connectors/microsoft/temporal/activities.ts

@@ -276,7 +283,8 @@ export async function markNodeAsSeen(connectorId: ModelId, internalId: string) {
  );

  if (!node) {
-    throw new Error(`Node ${internalId} not found`);
+    logger.warn(`Node ${internalId} not found`);


2 coms:

sure we wanna switch from breaking to logging?

would log a bit more in that case, at least the connector id; using e.g. a childLogger if you're familiar with the pattern?

yes, if you remove an item while doing a fullsync , since we don't stop anymore the workflow, it's normal that we fall into this case. The node should just be skipped.

Ok clear ! would there be a simple way to detect that? skip if not found because item removed, otherwise throw?
Also, is it fine with you to update the logging?

philipperolet · 2024-09-18T13:29:48Z

connectors/src/connectors/microsoft/temporal/activities.ts

@@ -310,7 +318,11 @@ export async function syncFiles({
  );

  if (!parent) {
-    throw new Error(`Unexpected: parent node not found: ${parentInternalId}`);
+    logger.warn(`Unexpected: parent node not found: ${parentInternalId}`);


same com as above

same same :-)

philipperolet · 2024-09-18T13:35:29Z

connectors/src/connectors/microsoft/index.ts

@@ -363,9 +363,13 @@ export class MicrosoftConnectorManager extends BaseConnectorManager<null> {
        internalId: id,
      }));

-    await MicrosoftRootResource.batchMakeNew(newResourcesBlobs);
+    const addedResources =
+      await MicrosoftRootResource.batchMakeNew(newResourcesBlobs);


if we just change the front end logic so here we only pass the 'changed' permissions, wouldn't that and the activities.ts changes below be enough?

taking care of removed folders too ofc

The removal is handled same way as before (deletion workflow is launched before starting new workflow/sending signal)

philipperolet · 2024-09-18T13:39:33Z

connectors/src/connectors/microsoft/temporal/workflows.ts

-  if (nodeIdsToSync === undefined) {
-    nodeIdsToSync = await getRootNodesToSync(connectorId);
-  }
+  setHandler(folderUpdatesSignal, (folderUpdates: FolderUpdatesSignal[]) => {


not sure I get why we add signal handling to the workflow; can't see where signals are sent and why they are needed VS sending the permission diff to setPermissions when doing add/remove?
Will ask you IRL so I better understand

We're using signalWithStart , which starts the workflow if it's not running, or send a signal if it does.

philipperolet

Thanks for the help understanding! Getting there, still have a few coms; happy to discuss
Also we had talked about handling the "multiple click" case in add / remove for which you apparently had a good, quick solution in mind? worth adding IMO

philipperolet · 2024-09-19T16:39:22Z

connectors/src/connectors/microsoft/temporal/workflows.ts

+    // If we get a signal, update the workflow state by adding/removing folder ids.
+    for (const { action, folderId } of folderUpdates) {
+      switch (action) {
+        case "added":


I have multiple questions here:

populateDeltas not executed for added node?

if node is added during the syncSucceed activity, node will be skipped?

if node is removed while it was already traversed by the while loop, then it will splice(-1,1) which will remove the last element? (not what we'd want)

maybe on restart we could somehow restart the workflow with the right nodeIdsToSync, so it would avoid issues above + the need to skip of previous comments?

also if folders were already synced how are they removed? I think you had told me but can't remember

populateDeltas is called directly in setPermissions. I don't know what's the use of the 2nd populateDelta in the workflow - that one will indeed not be called when adding nodes.

if node is added during the syncSucceed : I've added a check after the syncSucceeded, in case it happens

fixed the splice. on the other hand, I notice that "removed" signal is never sent ... I implemented it in a first version but finally only rely on the removal done in setPermissions(). btw i think we have the same issue with google connector. We should probably get rid of the microsoftDeletionWorkflow and handle all deletion inside the fullSync.

Refactored to include microsoftDeletionWorkflow inside fullSync. This also has the benefits of avoiding "parent node not found" logs, as it's done in sequence in the same workflow. However I think it's better to keep logs and not throw as a "missing parent" is not something recoverable by retrying the activity, and maight still happen with incremental sync running at the same time.

Ok but in that case with panic flag set to true right?
Because throwing, by making the activity retry, was triggering a monitor if the issue wasn't transient, that was prompting us to act; this is why I was inclined to keep the throw

philipperolet · 2024-09-19T16:40:06Z

connectors/src/connectors/microsoft/temporal/activities.ts

@@ -276,7 +283,8 @@ export async function markNodeAsSeen(connectorId: ModelId, internalId: string) {
  );

  if (!node) {
-    throw new Error(`Node ${internalId} not found`);
+    logger.warn(`Node ${internalId} not found`);


Ok clear ! would there be a simple way to detect that? skip if not found because item removed, otherwise throw?
Also, is it fine with you to update the logging?

philipperolet

Great! Left a final com, but approving already. Thanks 🙏

philipperolet

👍 😌

Update fullsync with signals to avoid complete resync

79fde6f

tdraier requested a review from philipperolet September 18, 2024 12:41

philipperolet reviewed Sep 18, 2024

View reviewed changes

philipperolet reviewed Sep 19, 2024

View reviewed changes

tdraier added 5 commits September 22, 2024 15:03

Check existing config before adding/removing nodes

ec9d212

logging, edge case

f9b8e07

removing only nodes to delete

f6477e9

more robust fullsync

e04d816

Merge branch 'main' into microsoft-update

254d981

tdraier requested a review from philipperolet September 23, 2024 08:16

philipperolet approved these changes Sep 24, 2024

View reviewed changes

philipperolet mentioned this pull request Sep 24, 2024

[Microsoft] Add/remove data: run fullsync only on newly added nodes (and scrub removed nodes) #7021

Closed

remove delete nodes from sync

83c2261

philipperolet approved these changes Sep 24, 2024

View reviewed changes

tdraier merged commit cce1d46 into main Sep 24, 2024
3 checks passed

tdraier deleted the microsoft-update branch September 24, 2024 09:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[connectors] feature: Update fullsync with signals to avoid complete resync #7494

[connectors] feature: Update fullsync with signals to avoid complete resync #7494

tdraier commented Sep 18, 2024 •

edited

Loading

philipperolet left a comment

philipperolet Sep 18, 2024

tdraier Sep 18, 2024

philipperolet Sep 19, 2024

philipperolet Sep 18, 2024

tdraier Sep 18, 2024

philipperolet Sep 18, 2024

philipperolet Sep 18, 2024

tdraier Sep 18, 2024

philipperolet Sep 18, 2024

tdraier Sep 18, 2024

philipperolet left a comment

philipperolet Sep 19, 2024

philipperolet Sep 19, 2024

philipperolet Sep 19, 2024

tdraier Sep 22, 2024

tdraier Sep 23, 2024

philipperolet Sep 24, 2024

philipperolet Sep 19, 2024

philipperolet left a comment

philipperolet left a comment

[connectors] feature: Update fullsync with signals to avoid complete resync #7494

[connectors] feature: Update fullsync with signals to avoid complete resync #7494

Conversation

tdraier commented Sep 18, 2024 • edited Loading

Description

Risk

Deploy Plan

philipperolet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipperolet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipperolet left a comment

Choose a reason for hiding this comment

philipperolet left a comment

Choose a reason for hiding this comment

tdraier commented Sep 18, 2024 •

edited

Loading