Fixed reverted PR: Choose optimal sync modes by default on UI #12770

dizel852 · 2022-05-11T14:20:16Z

What

Revert => Revert "Choose optimal sync modes by default on UI (#12411)" (#12583)

Fixed issue when sync modes were overrided in EditMode
Changed the order of available sync modes accordingly to the optimal sync mode strategy

Original description

Closes #9625
Set the optimal sync modes for Source and Destination by default on UI.

How

Choosing the mode by priority:

Incremental(cursor defined) => Append Dedup
Full Refresh => Overwrite
Incremental => Append
Full Refresh => Append

…12583)" This reverts commit 9789ffd.

krishnaglick

Does seem to preserve sync settings now! 🙌

edmundito

I tested this by creating a new connection and editing the sync modes in the connection.

edmundito · 2022-05-12T18:26:30Z

airbyte-webapp/src/views/Connection/ConnectionForm/formConfig.tsx

-        });
-
-        const supportedSyncModes = streamNode.stream.supportedSyncModes;
+export const calculateInitialCatalog = (


This function is resolving the initial schema, not necessarily calculating the initial catalog

Agree, that also confused me.
As I understood(if we look at the comments I removed) sometimes we can get supportedSyncModes: []
That's why we need to check the edit and creation modes supportedSyncModes.
I don't know how we can get empty supportedSyncModes.

Here's an example where we get empty supportedSyncModes and we have to handle it and set FULL_REFRESH as a default value

airbyte-webapp/src/views/Connection/ConnectionForm/formConfig.test.ts

edmundito

Changes 👍🏼

timroes · 2022-05-18T12:25:55Z

airbyte-webapp/src/views/Connection/ConnectionForm/calculateInitialCatalog.ts

+): SyncSchema => ({
+  streams: schema.streams.map<SyncSchemaStream>((apiNode, id) => {
+    const nodeWithId: SyncSchemaStream = { ...apiNode, id: id.toString() };
+    const nodeStream = verifySupportedSyncModes(nodeWithId);


This is the only place I am still a tiny bit concerned around. This method could change the existing Sync Mode when opening a connection in edit mode, if the previously set sync mode isn't available anymore. In that case the user would see a different sync mode in the UI, than the DB actually has, and also they would not be able to save it(??) because it's still the default values of the form?

@sherifnada Is there any case were a connector (e.g. due to an update) could lose a sync mode on an existing/saved connection's stream that was earlier supported? Is this something we'd need to cater for int he UI, or should that anyway never happen?

cc @edmundito @dizel852

@timroes

Is there any case were a connector (e.g. due to an update) could lose a sync mode on an existing/saved connection's stream that was earlier supported?

This is not defined behavior but I can't think of a case where there would be a good reason this edge case is needed. Is it possible to explicitly throw an error if that happens, or just leave it empty so the user has to fill it in?

If this is nothing we anticipate happening, I'd suggest we simply leave it empty and force the user to select a new one. @dizel852 Do you think we could still change that logic not to jump to full refresh overwrite if the actual configured sync doesn't exist, but leave the select box empty and invalid and force the user to select? (And ideally also add a unit test case for that scenario :D) Thanks

@timroes Yes, we can omit supportedSyncMode checking for edit mode.
But what should we verify it in creation mode?
If we will leave supportedSyncMode checking just for creation we will get a behavior:
https://www.loom.com/share/062406b7dd26417580a914ec6ffef0c6

As you can see, available sync modes rely on at least one (or default) value in 'supportedSyncMode'.

IMO One of the solutions - does not verify supportedSyncMode at all, neither in creation nor edit modes.
Do we get empty empty supportedSyncMode? - Okay, that means that our connection form is invalid since there are no available sync modes. So we just do not allow the user to create a connection.

Advantages: We don't try somehow fix the backend's response. Another question is why do we get such a response?
Disadvantages: More fixes 😅 Need to update our form validations and tests.

WDYT?

@dizel852 I agree with the part: that we should not try to fix a broken connector (like the end-2-end one) in the FE by overwriting it's available sync modes. I think we should simply not adjust them, and in this case not being able to create a connection with that connector. Otherwise we'd allow users to create a connection with a configuration the backend told us is invalid, which does not sound like a good idea.

Despite being more work I think we should make sure that we error the form in this case, i.e. allow for an undefined value in case there is no valid syncCode, check for that in our validation code, and make sure we're setting undefined if there's no valid configuration (either in getOptimalSyncMode or rather add another method call to calculateInitialCatalog for that.

With that said, we should still make sure our end-to-end testing connectors will not result in an invalid configuration that other connectors in practise can't end up. @sherifnada would you maybe able to take that to the connector team? Also if we change the UI here, without fixing the connectors I am not sure if we actually still can run end-to-end tests, thus it would be good if we could solve that somehow in the near future if possible :)

Despite being more work I think we should make sure that we error the form in this case, i.e. allow for an undefined value in case there is no valid syncCode, check for that in our validation code, and make sure we're setting undefined if there's no valid configuration (either in getOptimalSyncMode or rather add another method call to calculateInitialCatalog for that.

Ok, got it. Will update the implementation. 👍

timroes

I've tested it locally and looked through the code. Everything seems to be working (creation or editing) for me. I've tested with the GitHub source and postgres destination. I changed some values when creating the connection and validated that editing it, it will still be the same. I have one open question that I pinged Sherif on, that I'd like to get clarified before approving this.

teallarson · 2022-05-18T14:22:40Z

Wondering if we want to hold off on this until this issue is resolved: #12728
It appears to be different from the bug we saw prior to resolving the previous "optimal previous sync mode" bug, but in that same area of the code.

# Conflicts: # airbyte-webapp/src/views/Connection/ConnectionForm/formConfig.tsx

- fix Type

dizel852 · 2022-05-23T20:03:28Z

Finally resolved all merge conflicts 🤪 That was something...
Reminder: We agreed on our weekly sync not to change the logic regarding checking supportedSyncModes and leave it as is. At least for now. I will create an issue for that case. airbytehq/airbyte-internal-issues#637

@tealjulia If you don't mind, can we merge the PR before #12728
It's pretty hard to keep PR in sync with the master...

cc: @timroes

teallarson · 2022-05-24T14:04:55Z

I'm fine with that if it's been tested locally by others! I was more just curious if there might be an interaction between the two.

teallarson

I've tested it locally and it works as expected on Connection creation and editing (tested using Github source, changed sync modes, namespace format, turned streams off, etc.). I'm not sure where we're at with the questions above for the Connectors team, and given that this form has been finicky lately, this probably deserves a second approval before merging.

dizel852 · 2022-05-24T19:00:02Z

@tealjulia agree.
@timroes the last word is yours 😄

timroes

Code LGTM. Tested several scenarios, and it seems to work as we agreed on.

…ehq#12770) * Revert "Revert "Choose optimal sync modes by default on UI (airbytehq#12411)" (airbytehq#12583)" This reverts commit 9789ffd. * get optimal sync mode only in creation mode * sort sync mode options by priority * add tests for calculateInitialCatalog * remove duplicated tests * fixed one-line 'if' * move calculationInitialCatalog function to file and export it as default * - fix merge conflicts - fix Type * update tests

dizel852 added 6 commits May 10, 2022 20:32

Revert "Revert "Choose optimal sync modes by default on UI (#12411)" (#…

f22f37c

…12583)" This reverts commit 9789ffd.

get optimal sync mode only in creation mode

e0efa99

sort sync mode options by priority

38bba30

add tests for calculateInitialCatalog

20ddaac

Merge branch 'master' into vlad/fixed-optimal-sync-mode-after-revert

a7e79fa

remove duplicated tests

e518335

dizel852 added area/frontend area/platform issues related to the platform labels May 11, 2022

dizel852 requested review from edmundito, timroes, krishnaglick and teallarson May 11, 2022 14:20

dizel852 marked this pull request as ready for review May 11, 2022 14:29

dizel852 requested a review from a team as a code owner May 11, 2022 14:29

fixed one-line 'if'

19f7a0a

krishnaglick reviewed May 12, 2022

View reviewed changes

dizel852 self-assigned this May 12, 2022

edmundito reviewed May 12, 2022

View reviewed changes

dizel852 added 2 commits May 13, 2022 13:57

move calculationInitialCatalog function to file and export it as default

4f1c6da

Merge branch 'master' into vlad/fixed-optimal-sync-mode-after-revert

ce81e97

edmundito reviewed May 16, 2022

View reviewed changes

timroes reviewed May 18, 2022

View reviewed changes

dizel852 added 4 commits May 19, 2022 14:30

Merge branch 'master' into vlad/fixed-optimal-sync-mode-after-revert

9e422e0

Merge branch 'master' into vlad/fixed-optimal-sync-mode-after-revert

188d185

# Conflicts: # airbyte-webapp/src/views/Connection/ConnectionForm/formConfig.tsx

- fix merge conflicts

102ac9a

- fix Type

update tests

8fc8264

dizel852 added area/frontend Related to the Airbyte webapp and removed area/platform issues related to the platform labels May 24, 2022

teallarson approved these changes May 24, 2022

View reviewed changes

edmundito requested a review from timroes May 27, 2022 12:27

Merge branch 'master' into vlad/fixed-optimal-sync-mode-after-revert

8ca7640

github-actions bot added the area/platform issues related to the platform label May 31, 2022

timroes approved these changes May 31, 2022

View reviewed changes

dizel852 merged commit ff4b789 into master May 31, 2022

dizel852 deleted the vlad/fixed-optimal-sync-mode-after-revert branch May 31, 2022 13:36

octavia-squidington-iii mentioned this pull request May 31, 2022

Bump Airbyte version from 0.39.5-alpha to 0.39.6-alpha #13350

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed reverted PR: Choose optimal sync modes by default on UI #12770

Fixed reverted PR: Choose optimal sync modes by default on UI #12770

dizel852 commented May 11, 2022

krishnaglick left a comment

edmundito left a comment •

edited

Loading

edmundito May 12, 2022

dizel852 May 13, 2022

dizel852 May 15, 2022

edmundito left a comment

timroes May 18, 2022

sherifnada May 18, 2022

timroes May 18, 2022

dizel852 May 18, 2022 •

edited

Loading

timroes May 19, 2022

dizel852 May 19, 2022

timroes left a comment

teallarson commented May 18, 2022 •

edited

Loading

dizel852 commented May 23, 2022 •

edited

Loading

teallarson commented May 24, 2022

teallarson left a comment •

edited

Loading

dizel852 commented May 24, 2022

timroes left a comment

Fixed reverted PR: Choose optimal sync modes by default on UI #12770

Fixed reverted PR: Choose optimal sync modes by default on UI #12770

Conversation

dizel852 commented May 11, 2022

What

Original description

How

krishnaglick left a comment

Choose a reason for hiding this comment

edmundito left a comment • edited Loading

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

edmundito 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

dizel852 May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

teallarson commented May 18, 2022 • edited Loading

dizel852 commented May 23, 2022 • edited Loading

teallarson commented May 24, 2022

teallarson left a comment • edited Loading

Choose a reason for hiding this comment

dizel852 commented May 24, 2022

timroes left a comment

Choose a reason for hiding this comment

edmundito left a comment •

edited

Loading

dizel852 May 18, 2022 •

edited

Loading

teallarson commented May 18, 2022 •

edited

Loading

dizel852 commented May 23, 2022 •

edited

Loading

teallarson left a comment •

edited

Loading