Add Geography support to RouterService #17902

pmossman · 2022-10-12T18:38:39Z

What

RouterService selects a task queue based on a connection's Geography stored in the config database, rather than using environment variables.

How

Add a GeographyMapper interface that maps Geography enum values to String task queue names
Add a 'DefaultGeographyMapper' implementation that OSS will use. Idea here is that Cloud will provide it's own implementation with a '@primary' annotation so that Micronaut favors it over the default implementation.

Reading Order

Read my comments top to bottom as an initial guideline

davinchia · 2022-10-14T00:47:07Z

@pmossman is this ready for review?

pmossman · 2022-10-14T01:03:37Z

@davinchia yeah I think so!

pmossman · 2022-10-19T00:13:45Z

...e-commons-temporal/src/main/java/io/airbyte/commons/temporal/scheduling/GeographyMapper.java

+ * Copyright (c) 2022 Airbyte, Inc., all rights reserved.
+ */
+
+package io.airbyte.commons.temporal.scheduling;


This interface is in the airbyte-commons-temporal module, so that on the Cloud side, the new cloud-workers module can depend on airbyte-commons-temporal without needing a dependency on airbyte-workers.

I debated putting this interface in airbyte-commons-worker but based on the contents of the two modules, airbyte-commons-temporal seemed like a slightly better fit. Definitely open to other opinions here!

pmossman · 2022-10-19T00:14:45Z

airbyte-workers/src/main/java/io/airbyte/workers/temporal/scheduling/RouterService.java

@@ -0,0 +1,37 @@
+/*


This diff shows up as a brand new file because I moved the RouterService from /sync/ to /scheduling/` because it's used by the ConnectionManagerWorkflow, not the SyncWorkflow.

This diff also contains the new functionality of reading the connection's Geography from the database instead of from the environment variable

pmossman · 2022-10-19T00:16:16Z

...java/io/airbyte/workers/temporal/scheduling/activities/RouteToSyncTaskQueueActivityImpl.java


-    return new RouteToSyncTaskQueueOutput(taskQueueForConnectionId);
+      return new RouteToSyncTaskQueueOutput(taskQueueForConnectionId);
+    } catch (final IOException e) {


Now that the routerService queries the database, an IOException is possible. Throw a RetryableException if encountered

Remind me again, where do we catch the RetryableException? I can't look since as my IntelliJ is pooping out on me.

Asked here: https://airbytehq-team.slack.com/archives/C03AS1GAQV6/p1666217128557719

I think it extends RuntimeException which means Temporal knows to retry activities that fail with it as a cause, but it isn't super clear

pmossman · 2022-10-19T00:24:26Z

...yte-workers/src/main/java/io/airbyte/workers/temporal/scheduling/DefaultGeographyMapper.java

@@ -0,0 +1,37 @@
+/*


Here's the OSS implementation of the GeographyMapper. In Cloud, this class will be overridden with a @Replaces annotation

For my own understanding too, since I haven't had a chance to work much with micronaut (need to change that!).

How do the @replace and @primary annotations interact?

@davinchia They are two different options for the same result. Any bean marked @Primary will always win. @Replace is used to take the place of an existing bean of the same type. I haven't played with it, but I suspect that @Replace wouldn't work if no other bean exists of that type, but @Primary would. Generally, one is a bit more loose than the other, so it really depends on if we have more than 2 versions, as we won't want to mark two of the three of the same type with @Primary.

Sounds like we should default to @primary and use @replace on a needs-to basis

@davinchia That is my assessment as well.

pmossman · 2022-10-19T00:27:17Z

@jdpgrailsdev @davinchia I think this should be ready for review in tandem with the Cloud PR

jdpgrailsdev · 2022-10-19T14:36:01Z

airbyte-workers/src/main/java/io/airbyte/workers/temporal/scheduling/RouterService.java

+ */
+@Singleton
+@Slf4j
+@AllArgsConstructor


Nit: Our recommendation is to not use Lombok and instead create an explicit constructor with the beans to be injected. This allows us to use annotations on the fields should that become necessary.

@jdpgrailsdev for my knowledge, what does 'allows us to use annotations on the fields should that become necessary' mean?

(I understand and agree on not mixing constructor annotations from separate AOP frameworks for clarity/best practices.)

@davinchia With the Lombok generated constructor, there is no code for us to modify if we end up needing to add a @Qualifier annotation to specify which version of a singleton to inject, nor is there the ability for us to use @Value or @Property to inject configuration values via the constructor. Put another way, the Lombok generated constructor only works for constructor-based injection if we do not need to annotate any of the parameters in order for Micronaut to make the correct injection.

Got it. Thanks for explaining.

I think we should also start phasing out Lombok in our system for clarity. Does Micronaut have an @Slf4j annotation equivalent? I believe these two are the main Lombok annotations we use.

@davinchia No, not that I am aware of. It provides the configuration of the logging system, but the use of a logger is up to the developer. I'm not sure that the @Slf4j annotation really saves us a whole lot, as declaring a logger is a 1 line statement, much like the annotation.

Yeah. I think the benefit of the annotation is mostly not need to remember to type the long string and the hideous class import - all cosmetic.

MIcronaut docs do indicate some clashing of Lombok and Micronaut - let's get rid of Lombok!

jdpgrailsdev

davinchia · 2022-10-19T21:14:07Z

airbyte-config/config-models/src/main/java/io/airbyte/config/Configs.java

@@ -579,16 +579,6 @@ public interface Configs {
   */
  boolean shouldRunConnectionManagerWorkflows();

-  // Worker - Control Plane configs


davinchia · 2022-10-19T21:14:36Z

airbyte-workers/src/main/java/io/airbyte/workers/temporal/scheduling/RouterService.java

+  private final ConfigRepository configRepository;
+  private final GeographyMapper geographyMapper;
+
+  public RouterService(final ConfigRepository configRepository, final GeographyMapper geographyMapper) {


This is where Micronaut will inject the Cloud specific bean?

Yes. It will inject which ever implementation of the GeographyMapper wins. In OSS, this will be the default implementation (the only one present). In cloud, both are present on the classpath, but only the one provided by the cloud-worker library will win because of the Primary annotation.

davinchia · 2022-10-19T21:20:26Z

...java/io/airbyte/workers/temporal/scheduling/activities/RouteToSyncTaskQueueActivityImpl.java

-    return new RouteToSyncTaskQueueOutput(taskQueueForConnectionId);
+      return new RouteToSyncTaskQueueOutput(taskQueueForConnectionId);
+    } catch (final IOException e) {
+      throw new RetryableException(e);


nit: log.warn here?

davinchia · 2022-10-19T21:23:41Z

...yte-workers/src/main/java/io/airbyte/workers/temporal/scheduling/DefaultGeographyMapper.java

+public class DefaultGeographyMapper implements GeographyMapper {
+
+  @VisibleForTesting
+  static final String DEFAULT_SYNC_TASK_QUEUE = TemporalJobType.SYNC.name();


davinchia · 2022-10-19T21:29:46Z

...e-commons-temporal/src/main/java/io/airbyte/commons/temporal/scheduling/GeographyMapper.java

+ * Maps a {@link Geography} to a Temporal Task Queue that should be used to run syncs for the given
+ * Geography.
+ */
+public interface GeographyMapper {


Thinking out loud:

Would a better name not include Geography? Thinking about the future where we want the input to getTaskQueue to not be tied to geography. Maybe QueueMapper?

Landing what we have now and evolving this later is also okay to me.

davinchia · 2022-10-19T21:30:25Z

...workers/src/test/java/io/airbyte/workers/temporal/scheduling/DefaultGeographyMapperTest.java

+
+  /**
+   * If this test fails, it likely means that a new value was added to the {@link Geography} enum. A
+   * new entry must be added to {@link DefaultGeographyMapper#GEOGRAPHY_TASK_QUEUE_MAP} to get this


davinchia

Thanks @pmossman

Both the code comments and the PR comments made it very easy to review.

I have one medium comment about naming of the geography mapper. I have some smaller comments for my own understanding.

None of these are blocking. We can iterate on a follow up PR.

move geography map to a helper that can be overridden with a separate implementation in cloud format pmd fix import move geography mapper interface to airbyte-commons-temporal

* router service uses geography in database instead of env var move geography map to a helper that can be overridden with a separate implementation in cloud format pmd fix import move geography mapper interface to airbyte-commons-temporal * add DefaultGeographyMapper back in * remove all args constructor and extranneous import * rename GeographyMapper to TaskQueueMapper

github-actions bot added area/platform issues related to the platform area/worker Related to worker labels Oct 12, 2022

pmossman temporarily deployed to more-secrets October 12, 2022 19:22 Inactive

pmossman temporarily deployed to more-secrets October 12, 2022 20:47 Inactive

pmossman temporarily deployed to more-secrets October 13, 2022 00:08 Inactive

pmossman temporarily deployed to more-secrets October 13, 2022 18:00 Inactive

pmossman requested a review from davinchia October 13, 2022 22:48

pmossman marked this pull request as ready for review October 14, 2022 01:03

pmossman force-pushed the parker/router-service-using-db branch from 9bfe03b to 8cc89fb Compare October 18, 2022 20:18

pmossman temporarily deployed to more-secrets October 18, 2022 20:21 Inactive

pmossman force-pushed the parker/router-service-using-db branch 2 times, most recently from f021777 to 9fed5f0 Compare October 19, 2022 00:09

pmossman temporarily deployed to more-secrets October 19, 2022 00:11 Inactive

pmossman temporarily deployed to more-secrets October 19, 2022 00:24 Inactive

pmossman commented Oct 19, 2022

View reviewed changes

pmossman requested a review from jdpgrailsdev October 19, 2022 00:24

jdpgrailsdev reviewed Oct 19, 2022

View reviewed changes

jdpgrailsdev approved these changes Oct 19, 2022

View reviewed changes

pmossman force-pushed the parker/router-service-using-db branch from a5430be to c6b30ea Compare October 19, 2022 15:08

pmossman temporarily deployed to more-secrets October 19, 2022 15:10 Inactive

pmossman force-pushed the parker/router-service-using-db branch from c6b30ea to 9c79c5a Compare October 19, 2022 15:20

pmossman temporarily deployed to more-secrets October 19, 2022 15:22 Inactive

davinchia reviewed Oct 19, 2022

View reviewed changes

davinchia approved these changes Oct 19, 2022

View reviewed changes

pmossman temporarily deployed to more-secrets October 20, 2022 00:16 Inactive

pmossman force-pushed the parker/router-service-using-db branch from 39cf2ab to 0301e31 Compare October 20, 2022 00:45

pmossman temporarily deployed to more-secrets October 20, 2022 00:47 Inactive

pmossman force-pushed the parker/router-service-using-db branch from 0301e31 to 9a5ab5a Compare October 20, 2022 01:55

pmossman temporarily deployed to more-secrets October 20, 2022 01:57 Inactive

pmossman added 4 commits October 20, 2022 12:00

router service uses geography in database instead of env var

7a0eedd

move geography map to a helper that can be overridden with a separate implementation in cloud format pmd fix import move geography mapper interface to airbyte-commons-temporal

add DefaultGeographyMapper back in

2c87d34

remove all args constructor and extranneous import

05518c9

rename GeographyMapper to TaskQueueMapper

53045c1

pmossman force-pushed the parker/router-service-using-db branch from 9a5ab5a to 53045c1 Compare October 20, 2022 19:00

pmossman temporarily deployed to more-secrets October 20, 2022 19:02 Inactive

pmossman temporarily deployed to more-secrets October 20, 2022 20:10 Inactive

pmossman temporarily deployed to more-secrets October 20, 2022 20:14 Inactive

pmossman merged commit 4e236b5 into master Oct 20, 2022

pmossman deleted the parker/router-service-using-db branch October 20, 2022 21:48

octavia-squidington-iii mentioned this pull request Oct 21, 2022

Bump Airbyte version from 0.40.16 to 0.40.17 #18316

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Geography support to RouterService #17902

Add Geography support to RouterService #17902

pmossman commented Oct 12, 2022 •

edited

Loading

davinchia commented Oct 14, 2022

pmossman commented Oct 14, 2022

pmossman Oct 19, 2022

pmossman Oct 19, 2022

pmossman Oct 19, 2022

davinchia Oct 19, 2022

pmossman Oct 20, 2022

pmossman Oct 19, 2022

davinchia Oct 19, 2022

jdpgrailsdev Oct 19, 2022

davinchia Oct 19, 2022

jdpgrailsdev Oct 19, 2022

pmossman commented Oct 19, 2022

jdpgrailsdev Oct 19, 2022

davinchia Oct 19, 2022 •

edited

Loading

jdpgrailsdev Oct 19, 2022

davinchia Oct 19, 2022

jdpgrailsdev Oct 19, 2022

davinchia Oct 20, 2022

jdpgrailsdev left a comment

davinchia Oct 19, 2022

davinchia Oct 19, 2022

jdpgrailsdev Oct 19, 2022

davinchia Oct 19, 2022

davinchia Oct 19, 2022

davinchia Oct 19, 2022

davinchia Oct 19, 2022

davinchia left a comment

Add Geography support to RouterService #17902

Add Geography support to RouterService #17902

Conversation

pmossman commented Oct 12, 2022 • edited Loading

What

How

Reading Order

davinchia commented Oct 14, 2022

pmossman commented Oct 14, 2022

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

pmossman commented Oct 19, 2022

Choose a reason for hiding this comment

davinchia Oct 19, 2022 • 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

Choose a reason for hiding this comment

jdpgrailsdev 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

davinchia left a comment

Choose a reason for hiding this comment

pmossman commented Oct 12, 2022 •

edited

Loading

davinchia Oct 19, 2022 •

edited

Loading