[GOBBLIN-1901] Define MultiActiveLeaseArbiter Decorator to Model Failed Lease Completion #3765

umustafi · 2023-09-09T00:19:15Z

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

My PR addresses the following Gobblin JIRA issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-1901

Description

Here are some details about my PR, including screenshots (if applicable):
Creates MysqlMultiActiveLeaseAribterTestingDecorator class used to model scenarios where a lease owner fails to complete a lease successfully intermittently (representing a variety of slowness or failure cases that can result on the participant side, network connectivity, or database).
It will fail on calls to {@link MysqlMultiActiveLeaseArbiter.recordLeaseSuccess()} where a deterministic function of the lease obtained timestamp matches a bitmask of the host. Ideally, each participant should fail on different calls (with limited overlap if we want to test that). See java doc for more details.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:
WIP (will add in second iteration once methodology is agreed upon)

Commits

My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

phet · 2023-09-12T00:51:48Z

gobblin-utility/src/main/java/org/apache/gobblin/util/HostUtils.java

+
+  /**
+   * Given a host name return an optional containing the InetAddress object if one can be constructed for the input
+   * // TODO: provide details about expected hostName format


may be no need to stipulate, given the type

... perhaps rename the function to getInetAddressForHostName?

phet · 2023-09-12T00:59:49Z

gobblin-api/src/main/java/org/apache/gobblin/configuration/ConfigurationKeys.java

@@ -103,6 +103,11 @@ public class ConfigurationKeys {
  public static final String DEFAULT_SCHEDULER_LEASE_DETERMINATION_STORE_DB_TABLE = "gobblin_scheduler_lease_determination_store";
  // Refers to the event we originally tried to acquire a lease which achieved `consensus` among participants through
  // the database
+  public static final String MULTI_ACTIVE_LEASE_ARBITER_HOST_TO_BIT_MASK_MAP = MYSQL_LEASE_ARBITER_PREFIX + ".hostToBitMaskMap";


since this is not interpreted by the MysqlMALeaseArbiter, it shouldn't borrow that class's config prefix. let's make a prefix just for the class reading this config. (that will clue in config maintainers that it's unnecessary once the class itself is no longer being used.)

phet · 2023-09-12T01:15:35Z

...c/main/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiterTestingDecorator.java

+ * host. If the bitwise AND comparison to the host bit mask equals the bitmask we fail the call.
+ */
+@Slf4j
+public class MysqlMultiActiveLeaseArbiterTestingDecorator extends MysqlMultiActiveLeaseArbiter {


the name TestingDecorator suggests nothing about behavior. Decorator seems a relevant pattern, and suggests reusability, but it's more important to name how/why it decorates than to state vaguely that decoration is afoot.

how about FailureInjectingMALeaseArbiter or FailureInjectingMALeaseArbiterDecorator?

Also, on the subject of reuse, why base this specifically on MysqlMALeaseArbiter, rather the MALeaseArbiter interface/base class? as currently written, this is not truly a decorator, but rather an extension to MysqlMALA. (in java a decorator would implement an interface, but almost never extend a concrete derived class that already implements it.)

to be a decorator, the ctor should either take a MALeaseArbiter instance to delegate to or else create one internally. here, I suggest the latter, which means this class (itself named within config) should itself read its own (prefixed) config to learn the class name it should internally create an instance of.

phet · 2023-09-12T01:24:13Z

...c/main/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiterTestingDecorator.java

+public class MysqlMultiActiveLeaseArbiterTestingDecorator extends MysqlMultiActiveLeaseArbiter {
+  private final int bitMaskLength;
+  private final int numHosts;
+  private final HashMap<Integer, Integer> hostIdToBitMask = new HashMap();


shouldn't there be only one MALA instance per host? if so, it shouldn't need to worry about any host bitmask but its own, so probably an Optional<Integer>.

phet · 2023-09-12T01:24:42Z

gobblin-api/src/main/java/org/apache/gobblin/configuration/ConfigurationKeys.java

@@ -103,6 +103,11 @@ public class ConfigurationKeys {
  public static final String DEFAULT_SCHEDULER_LEASE_DETERMINATION_STORE_DB_TABLE = "gobblin_scheduler_lease_determination_store";
  // Refers to the event we originally tried to acquire a lease which achieved `consensus` among participants through
  // the database
+  public static final String MULTI_ACTIVE_LEASE_ARBITER_HOST_TO_BIT_MASK_MAP = MYSQL_LEASE_ARBITER_PREFIX + ".hostToBitMaskMap";
+  public static final String MULTI_ACTIVE_LEASE_ARBITER_BIT_MASK_LENGTH = MYSQL_LEASE_ARBITER_PREFIX + ".bitMaskLength";


usually BIT_MASK is one word, so BITMASK and bitmask

phet · 2023-09-12T01:44:32Z

...c/main/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiterTestingDecorator.java

+  /**
+   * Retrieve the host id as a number between 1 through `numHosts` by using the host address's hashcode.
+   * @return
+   */
+  protected int getHostIdFromAddress(InetAddress address) {
+      return (address.hashCode() % numHosts) + 1;
+  }


I don't understand the concept of host ID... please explain.

I'd imagine each host would be identified by its specific hostname and the configured bitmask it should use keyed by that name

phet · 2023-09-12T01:45:30Z

...c/main/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiterTestingDecorator.java

+   * @param bitmask 4-bit binary integer used to compare against modified lease acquisition timestamp
+   * @return true if the host should fail the lease completion attempt
+   */
+  protected static boolean shouldFailLeaseCompletionAttempt(LeaseObtainedStatus status, int bitmask,


likely should be @VisibleForTesting

phet · 2023-09-12T01:46:13Z

...c/main/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiterTestingDecorator.java

+  protected static boolean shouldFailLeaseCompletionAttempt(LeaseObtainedStatus status, int bitmask,
+      int bitMaskLength) {
+    // Convert event timestamp to binary
+    Long.toString(status.getLeaseAcquisitionTimestamp()).getBytes();


remove this (since not assigned)

phet · 2023-09-12T01:52:25Z

...c/main/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiterTestingDecorator.java

+    String scatteredBinaryString = scatterBinaryStringBits(binaryString);
+    // Take last `bitMaskLength`` bits of the string
+    int length = scatteredBinaryString.length();
+    String shortenedBinaryString = scatteredBinaryString.substring(length-bitMaskLength, length);


let's do binary entirely with numbers (no strings). e.g.:

boolean shouldFail = (scatterTimestamp(ts) & bitmaskBits) != 0

phet · 2023-09-12T02:04:19Z

...c/main/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiterTestingDecorator.java

+ * success with the following methodology. We take the binary representation of the lease obtained timestamp, scatter
+ * its bits through bit interleaving of the first and second halves of the binary representation to differentiate
+ * behavior of consecutive timestamps, and compare the last N digits (determined through config) to the bit mask of the


I really like the idea of scattering (to avoid long runs of on/off). while I have very little background in number theory, the idea of interleaving high-order with low-order bits intuitively concerns me... so not sure if I'm missing something.

here's my logic: when representing millis as a java long, we can essentially consider the high-order bits to be constants, since the lowest of the long's high bits [32:63] only changes once every 10^9 secs. after interleaving, wouldn't odd position bits nearly never change, while even-positions would. the assignment:

host0:0001,host1:0010,host2:0100,host3:1000

would give drastically different probability for host1 and host3, than for host0, and host2--right?

instead I suggest bit "scattering" by passing the number through a hash digest, like MD5 or SHA1

Urmi Mustafi added 3 commits September 8, 2023 17:13

Define multi-active lease arbiter testing decorator

c840341

fix build error

3387481

Add configuration for num hosts

43eae5f

phet reviewed Sep 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GOBBLIN-1901] Define MultiActiveLeaseArbiter Decorator to Model Failed Lease Completion #3765

[GOBBLIN-1901] Define MultiActiveLeaseArbiter Decorator to Model Failed Lease Completion #3765

[GOBBLIN-1901] Define MultiActiveLeaseArbiter Decorator to Model Failed Lease Completion #3765

Are you sure you want to change the base?

[GOBBLIN-1901] Define MultiActiveLeaseArbiter Decorator to Model Failed Lease Completion #3765

Conversation

JIRA

Description

Tests

Commits

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