[GitHub] [gobblin] aplex commented on a change in pull request #3333: [GOBBLIN-1491] Add option to mark up/down D2 servers for gobblin service on leadership change

[GitBox <gi...@apache.org>]

aplex commented on a change in pull request #3333:
URL: https://github.com/apache/gobblin/pull/3333#discussion_r670908530



##########
File path: gobblin-api/src/main/java/org/apache/gobblin/service/ServiceConfigKeys.java
##########
@@ -35,6 +35,7 @@
   public static final String GOBBLIN_SERVICE_GIT_CONFIG_MONITOR_ENABLED_KEY = GOBBLIN_SERVICE_PREFIX + "gitConfigMonitor.enabled";
   public static final String GOBBLIN_SERVICE_DAG_MANAGER_ENABLED_KEY = GOBBLIN_SERVICE_PREFIX + "dagManager.enabled";
   public static final String GOBBLIN_SERVICE_JOB_STATUS_MONITOR_ENABLED_KEY = GOBBLIN_SERVICE_PREFIX + "jobStatusMonitor.enabled";
+  public static final String GOBBLIN_SERVICE_DELAYED_START = GOBBLIN_SERVICE_PREFIX + "d2.delayedStart";

Review comment:
       Can we name this after intention, rather than implementation? Looks like what we want to configure here is "send d2 requests only to master node". You can also add a comment when this is needed - if d2 randomly balances requests across all servers, some of the write requests will fail.

##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/D2Announcer.java
##########
@@ -0,0 +1,17 @@
+package org.apache.gobblin.service.modules.core;
+
+
+/**
+ * Interface for marking up/down D2 servers on gobblin service startup. This is only required if using delayed announcement.
+ */
+public interface D2Announcer {
+  /**
+   * Mark up D2 servers
+   */
+  void markUpServers();

Review comment:
       Is it marking all servers as "up" or just the current one?

##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GobblinServiceManager.java
##########
@@ -191,6 +191,7 @@
 
   protected Optional<HelixLeaderState> helixLeaderGauges;
 
+  public D2Announcer d2Announcer;

Review comment:
       How is it initialized? If it is optional, and is going to be set through dependency injection, it needs  @Inject(optional = true) attribute.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org