phet commented on code in PR #3765:
URL: https://github.com/apache/gobblin/pull/3765#discussion_r1322254090
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiterTestingDecorator.java:
##########
@@ -0,0 +1,206 @@
+package org.apache.gobblin.runtime.api;
+
+import com.google.common.base.Optional;
+import com.typesafe.config.Config;
+import java.io.IOException;
+import java.net.Inet6Address;
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+import java.util.HashMap;
+import javax.inject.Inject;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.gobblin.configuration.ConfigurationKeys;
+import org.apache.gobblin.util.ConfigUtils;
+import org.apache.gobblin.util.HostUtils;
+
+
+/**
+ * This class is a decorator for {@link MysqlMultiActiveLeaseArbiter} used to
model scenarios where a lease owner fails
+ * to complete a lease 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 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). We use a deterministic method of
failing some calls to complete a lease
+ * 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
+ * host. If the bitwise AND comparison to the host bit mask equals the bitmask
we fail the call.
+ */
+@Slf4j
+public class MysqlMultiActiveLeaseArbiterTestingDecorator extends
MysqlMultiActiveLeaseArbiter {
Review Comment:
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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
