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

Skip validation when previous state is empty due to reset #20585

Merged
merged 14 commits into from
Jan 3, 2023

Conversation

jdpgrailsdev
Copy link
Contributor

What

Fix an edge case related to resets and syncs where at least one stream doesn't produce any data. The specific scenario plays out something like this:

  • New connection created
  • Connection is reset before any state is ever saved
    • This saves the state as LEGACY with a value of {"state":{}}
  • Sync is attempted with more than one configured stream
  • State save fails due to the validation on migration from legacy to per-stream state because not all streams produced data

How

  • Ignore state migration validation if previous state is LEGACY AND is empty. In this case, we don't care if we overwrite the existing state, as it has nothing in it and we don't risk losing state.

Recommended reading order

  1. PersistStateActivityImpl.java

Tests

  • Added unit test to verify expected behavior
  • All other tests pass
  • Project builds locally

@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/worker Related to worker labels Dec 16, 2022
final AirbyteApiClient airbyteApiClient1 = mock(AirbyteApiClient.class);
final StateApi stateApi1 = mock(StateApi.class);
final ConnectionState connectionState = mock(ConnectionState.class);
Mockito.lenient().when(connectionState.getStateType()).thenReturn(ConnectionStateType.LEGACY);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we bypass strict stubbing? Do we need those mock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benmoriceau I am not sure why this test has to do this. It is already creating lenient mocks and I originally attempted to not do that, but the tests fail without marking them lenient. In particular, these mocks are necessary, as we want to test the case where the previous state returned by the call to the state API returns a legacy state with an empty state JSON object for the data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, let's not address that in this PR

@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 16, 2022 17:45 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 16, 2022 17:45 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 16, 2022 21:35 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 16, 2022 21:36 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 17, 2022 00:34 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 17, 2022 00:34 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 19, 2022 14:39 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 19, 2022 14:40 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 19, 2022 21:50 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 19, 2022 21:50 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 19, 2022 21:57 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 19, 2022 21:57 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 19, 2022 22:00 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 19, 2022 22:01 — with GitHub Actions Inactive
@jdpgrailsdev
Copy link
Contributor Author

@benmoriceau I discovered a failing acceptance test which lead me to add an additional null check. There is a scenario where the ConnectionState object may be non-null, but the state object stored within it is null. The code has been refactored a bit to make it easier to read. Specifically, take a look at the isStateEmpty method in PersistStateActivityImpl. I also added tests to cover the three different null/empty state scenarios. Please re-review at your convenience.

@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 20, 2022 14:22 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets December 20, 2022 14:22 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets January 3, 2023 14:10 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets January 3, 2023 14:11 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets January 3, 2023 14:27 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets January 3, 2023 14:27 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets January 3, 2023 15:28 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets January 3, 2023 15:29 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets January 3, 2023 16:34 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets January 3, 2023 16:35 — with GitHub Actions Inactive
@jdpgrailsdev jdpgrailsdev merged commit c3987a9 into master Jan 3, 2023
@jdpgrailsdev jdpgrailsdev deleted the jonathan/persist-empty-state branch January 3, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform area/worker Related to worker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants