feat: return whether configuration was updated as part of api response #21466

pedroslopez · 2023-01-16T23:11:00Z

What

Returns a new connectorConfigurationUpdated boolean as part of the job metadata in synchronous job responses.
This will allow us to give the user the appropriate feedback / fetch the updated config on the frontend when jobs like check emit an updated configuration.

I opted to do this on the worker side and send the output rather than comparing configs on the api handler since it actually turned out to be simpler than dealing with hydrating the config with secrets and other things that came up with that approach. Since the worker knows what was the initial config, doing the comparison on that side is pretty straightforward.

We only really need this for check at the moment, but since any jobs that use configs can update configs (e.g. discover), this was added across all synchronous jobs. This also made it easier to pass this info along regardless of success/failure statues.

close #20913

Example response for a check that updated config:

{
  "status": "succeeded",
  "jobInfo": {
    "id": "502e0205-e178-4890-9ba6-ba02d8265091",
    "configType": "check_connection_source",
    "configId": "Optional[ef69ef6e-aa7f-4af1-a01d-ef775033524e]",
    "createdAt": 1673917879434,
    "endedAt": 1673917893148,
    "succeeded": true,
    "connectorConfigurationUpdated": true,
    "logs": {
      "logLines": [
        "2023-01-17 01:11:21 \u001b[32mINFO\u001b[m i.a.c.i.LineGobbler(voidCall):114 - ----- START CHECK -----",
        "...",
      ]
    }
  }
}

...and for one that didn't:

{
  "status": "succeeded",
  "message": "Source config: ContinuousFeedConfig{maxMessages=100, seed=0, messageIntervalMs=Optional.empty, mockCatalog=io.airbyte.protocol.models.AirbyteCatalog@56e07a08[streams=[io.airbyte.protocol.models.AirbyteStream@35d6ca49[name=data_stream,jsonSchema={\"type\":\"object\",\"properties\":{\"column1\":{\"type\":\"string\"},\"column2\":{\"type\":\"array\",\"items\":{\"type\":\"string\"}},\"column3\":{\"type\":\"number\"}}},supportedSyncModes=[full_refresh],sourceDefinedCursor=<null>,defaultCursorField=[],sourceDefinedPrimaryKey=[],namespace=<null>,additionalProperties={}], io.airbyte.protocol.models.AirbyteStream@3b9d6699[name=stream2,jsonSchema={\"type\":\"object\",\"properties\":{\"field1\":{\"type\":\"object\",\"properties\":{}}}},supportedSyncModes=[full_refresh],sourceDefinedCursor=<null>,defaultCursorField=[],sourceDefinedPrimaryKey=[],namespace=<null>,additionalProperties={}]],additionalProperties={}]}",
  "jobInfo": {
    "id": "bc0baed9-550c-43b0-8051-72184a6a3aef",
    "configType": "check_connection_source",
    "configId": "Optional[d53f9084-fa6b-4a5a-976c-5b8392f4ad8a]",
    "createdAt": 1673917270865,
    "endedAt": 1673917305828,
    "succeeded": true,
    "connectorConfigurationUpdated": false,
    "logs": {
      "logLines": [
        "2023-01-17 01:01:16 \u001b[32mINFO\u001b[m i.a.c.i.LineGobbler(voidCall):114 - ----- START CHECK -----",
        "...",
      ]
    }
  }
}

How

Added connectorConfigurationUpdated as part of the ConnectorJobOutput
Check / Discover workers now set the above appropriately if they processed a config message and this config message updated the configuration (if the config message contains the same configuration that was set as the job input, connectorConfigurationUpdated would be false)
Set connectorConfigurationUpdated as part of the SynchronousJobMetadata
Added connectorConfigUpdated to the SynchronousJobRead api schema (and pass value rom above)

github-actions · 2023-01-17T00:11:07Z

Airbyte Code Coverage

File	Coverage [53.68%]	❌
JobConverter.java	98.45%	🍏
DefaultSynchronousSchedulerClient.java	81.69%	🍏
SynchronousJobMetadata.java	40.27%	❌
SynchronousResponse.java	38.68%	❌
WorkerUtils.java	36.69%	❌
DefaultCheckConnectionWorker.java	0%	❌
DefaultDiscoverCatalogWorker.java	0%	❌

Total Project Coverage	26.74%	🍏

pedroslopez · 2023-01-17T00:23:17Z

I'm not quite sure what to do with this coverage comment 😅 I don't think my changes made it go down this much.
Also, I'm pretty sure the coverage report on DefaultCheckConnectionWorker and DefaultDiscoverCatalogWorker is wrong - each of these do have tests.

pedroslopez · 2023-01-16T23:12:53Z

airbyte-server/src/main/java/io/airbyte/server/scheduler/DefaultSynchronousSchedulerClient.java

-      final TemporalResponse<U> temporalResponse = executor.get();
-      final Optional<U> jobOutput = temporalResponse.getOutput();
+      final TemporalResponse<ConnectorJobOutput> temporalResponse = executor.get();
+      final Optional<ConnectorJobOutput> jobOutput = temporalResponse.getOutput();


Removed some generics since they weren't necessary (each job returns a ConnectorJobOutput) and were making it harder to work with

pedroslopez · 2023-01-16T23:14:29Z

...-server/src/test/java/io/airbyte/server/scheduler/DefaultSynchronousSchedulerClientTest.java

-    @SuppressWarnings(UNCHECKED)
-    @Test
-    void testExecuteMappedOutput() {
-      final UUID sourceDefinitionId = UUID.randomUUID();
-      final Supplier<TemporalResponse<Integer>> function = mock(Supplier.class);
-      final Function<Integer, String> mapperFunction = Object::toString;
-      when(function.get()).thenReturn(new TemporalResponse<>(42, createMetadata(true)));
-
-      final ConnectorJobReportingContext jobContext = new ConnectorJobReportingContext(UUID.randomUUID(), SOURCE_DOCKER_IMAGE);
-      final SynchronousResponse<String> response = schedulerClient
-          .execute(ConfigType.DISCOVER_SCHEMA, jobContext, sourceDefinitionId, function, mapperFunction, WORKSPACE_ID);
-
-      assertNotNull(response);
-      assertEquals("42", response.getOutput());
-      assertEquals(ConfigType.DISCOVER_SCHEMA, response.getMetadata().getConfigType());
-      assertTrue(response.getMetadata().getConfigId().isPresent());
-      assertEquals(sourceDefinitionId, response.getMetadata().getConfigId().get());
-      assertTrue(response.getMetadata().isSucceeded());
-      assertEquals(LOG_PATH, response.getMetadata().getLogPath());
-    }
-


this test was doing the same thing as the one above (testExecuteJobSuccess) - the only difference was the one above's mapperFunction returned the same parameter it was given.

pedroslopez · 2023-01-16T23:14:56Z

...-server/src/test/java/io/airbyte/server/scheduler/DefaultSynchronousSchedulerClientTest.java

+      final Supplier<TemporalResponse<ConnectorJobOutput>> function = mock(Supplier.class);
+      final Function<ConnectorJobOutput, UUID> mapperFunction = ConnectorJobOutput::getDiscoverCatalogId;


Changes required due to removing the generics - test now actually have to pass a ConnectorJobOutput

alafanechere

Nice!

alafanechere

Nice!

evantahler · 2023-01-18T00:11:32Z

airbyte-api/src/main/openapi/config.yaml

@@ -4200,6 +4200,9 @@ components:
          format: int64
        succeeded:
          type: boolean
+        connectorConfigurationUpdated:


nit: Could connectorConfigurationUpdated and didUpdateConfiguration both be the same name? Perhaps didUpdateConnectorConfiguration works in both cases?

I wanted to use didUpdateConnectorConfiguration, but PMD or some other check yelled at me because it wanted the get method on SynchronousJobMetadata to start with is :(

I can change the ConnectorJobOutput to be connectorConfigurationUpdated though

Updated in 37b734d

evantahler · 2023-01-18T00:12:48Z

airbyte-commons-worker/src/main/java/io/airbyte/workers/WorkerUtils.java

+  public static Boolean getDidControlMessageChangeConfig(final JsonNode initialConfigJson, final AirbyteControlConnectorConfigMessage configMessage) {
+    final Config newConfig = configMessage.getConfig();
+    final JsonNode newConfigJson = Jsons.jsonNode(newConfig);
+    return !initialConfigJson.equals(newConfigJson);


~~This is OK, but calling out that this would would fail if the config JSON was re-ordered by the connector. However, the connector shouldn't just re-order it's config without a good reason...~~

Actually, jackson.equals() should be positionally independant!

mfsiega-airbyte · 2023-01-20T10:32:13Z

@pedroslopez just a note - the intention of the code coverage comment is just to prompt you to consider whether you've sufficiently tested your changes here, and provide some signal.

As far as why it doesn't know that those files do have test coverage - the workers themselves live in airbyte-commons-workers, but the tests live in airbyte-workers. I'd guess that's what's confusing the coverage reporter.

pedroslopez added 7 commits January 11, 2023 16:57

wip: return whether configuration was updated

c275ea0

Merge branch 'master' into pedroslopez/config-updated-api

ae02e55

updated outputs working

e156156

fix pmd

62ef865

update description, format

ae8bfb0

add didUpdateConfiguration to metadata, rm unneeded generics

401d21e

add didUpdateConfiguration to api response

7365188

octavia-squidington-iv added area/api Related to the api area/documentation Improvements or additions to documentation area/platform issues related to the platform area/server area/worker Related to worker labels Jan 16, 2023

pedroslopez temporarily deployed to more-secrets January 16, 2023 23:12 — with GitHub Actions Inactive

pedroslopez temporarily deployed to more-secrets January 16, 2023 23:13 — with GitHub Actions Inactive

update name to fix pmd

8244fc1

pedroslopez temporarily deployed to more-secrets January 16, 2023 23:38 — with GitHub Actions Inactive

pedroslopez temporarily deployed to more-secrets January 16, 2023 23:39 — with GitHub Actions Inactive

not required

6be8708

pedroslopez temporarily deployed to more-secrets January 16, 2023 23:58 — with GitHub Actions Inactive

pedroslopez changed the title ~~Pedroslopez/config updated api~~ feat: return whether configuration was updated as part of api response Jan 17, 2023

pedroslopez commented Jan 17, 2023

View reviewed changes

pedroslopez marked this pull request as ready for review January 17, 2023 00:23

pedroslopez requested review from a team and evantahler January 17, 2023 00:25

alafanechere approved these changes Jan 17, 2023

View reviewed changes

evantahler approved these changes Jan 18, 2023

View reviewed changes

rename to match api response

37b734d

pedroslopez temporarily deployed to more-secrets January 18, 2023 17:19 — with GitHub Actions Inactive

remove unused field

269cdcc

pedroslopez temporarily deployed to more-secrets January 18, 2023 17:21 — with GitHub Actions Inactive

match naming

b82c872

pedroslopez enabled auto-merge (squash) January 18, 2023 17:23

pedroslopez temporarily deployed to more-secrets January 18, 2023 17:24 — with GitHub Actions Inactive

pedroslopez merged commit b24d575 into master Jan 18, 2023

pedroslopez deleted the pedroslopez/config-updated-api branch January 18, 2023 18:00

octavia-squidington-iii mentioned this pull request Jan 24, 2023

Bump Airbyte version from 0.40.28 to 0.40.29 #21767

Merged

pedroslopez mentioned this pull request Jan 26, 2023

🪟🎨 Clean up connector form form controls #21698

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: return whether configuration was updated as part of api response #21466

feat: return whether configuration was updated as part of api response #21466

pedroslopez commented Jan 16, 2023 •

edited

Loading

github-actions bot commented Jan 17, 2023 •

edited

Loading

pedroslopez commented Jan 17, 2023

pedroslopez Jan 16, 2023

pedroslopez Jan 16, 2023

pedroslopez Jan 16, 2023

alafanechere left a comment

alafanechere left a comment

evantahler Jan 18, 2023

pedroslopez Jan 18, 2023

pedroslopez Jan 18, 2023

evantahler Jan 18, 2023

mfsiega-airbyte commented Jan 20, 2023

		final Supplier<TemporalResponse<ConnectorJobOutput>> function = mock(Supplier.class);
		final Function<ConnectorJobOutput, UUID> mapperFunction = ConnectorJobOutput::getDiscoverCatalogId;

feat: return whether configuration was updated as part of api response #21466

feat: return whether configuration was updated as part of api response #21466

Conversation

pedroslopez commented Jan 16, 2023 • edited Loading

What

How

github-actions bot commented Jan 17, 2023 • edited Loading

Airbyte Code Coverage

pedroslopez commented Jan 17, 2023

pedroslopez Jan 16, 2023

Choose a reason for hiding this comment

pedroslopez Jan 16, 2023

Choose a reason for hiding this comment

pedroslopez Jan 16, 2023

Choose a reason for hiding this comment

alafanechere left a comment

Choose a reason for hiding this comment

alafanechere left a comment

Choose a reason for hiding this comment

evantahler Jan 18, 2023

Choose a reason for hiding this comment

pedroslopez Jan 18, 2023

Choose a reason for hiding this comment

pedroslopez Jan 18, 2023

Choose a reason for hiding this comment

evantahler Jan 18, 2023

Choose a reason for hiding this comment

mfsiega-airbyte commented Jan 20, 2023

pedroslopez commented Jan 16, 2023 •

edited

Loading

github-actions bot commented Jan 17, 2023 •

edited

Loading