ISSUE-640 : Fix URL failover mechanism in Schema Registry Client #664

raju-saravanan · 2020-02-06T14:45:40Z

Fixes #640

guruchai

Can you put a doc stitching up the URLSelectors, RetryPolicy and what it means to use of them?

guruchai · 2020-02-11T13:25:39Z

...istry/common/src/main/java/com/hortonworks/registries/schemaregistry/retry/RetryContext.java

+
+import java.util.Objects;
+
+public class RetryContext<T> {


Java Doc please

Create a private constructor to make sure that the instance can only be created through the builder.

Ah, I missed this. Thanks for pointing out.

guruchai · 2020-02-11T13:26:44Z

...istry/common/src/main/java/com/hortonworks/registries/schemaregistry/retry/RetryManager.java

+        Class<? extends RuntimeException> exceptionClass = retryContext.retryOnException();
+        RuntimeException exception = null;
+
+        System.out.println("Policy : " + policy);


This should be deleted.

guruchai · 2020-02-11T13:31:11Z

...va/com/hortonworks/registries/schemaregistry/retry/policy/ExponentialBackoffRetryPolicy.java

+
+import java.util.Map;
+
+public class ExponentialBackoffRetryPolicy extends RetryPolicy {


JavaDoc please

guruchai · 2020-02-11T13:37:17Z

...on/src/main/java/com/hortonworks/registries/schemaregistry/retry/policy/NOOPRetryPolicy.java

+
+public class NOOPRetryPolicy extends RetryPolicy {
+
+    public NOOPRetryPolicy() {


NOOPRetryPolicy is effectively no retry, if you are doing right away. Isnt it?

guruchai · 2020-02-11T13:38:12Z

...ent/src/main/java/com/hortonworks/registries/schemaregistry/client/SchemaRegistryClient.java

+    private static final String RETRY_STRATEGY_CONFIG_KEY = "config";
+
+    private static final RetryManager RETRY_MANAGER = new RetryManager();
+    private static final String DEFAULT_RETRY_STRATEGY_CLASS = NOOPRetryPolicy.class.getCanonicalName();


I think you should have FixedRetry/Exponential as a default policy.

Many of the existing customers would be handling retry on the application side, in order to changes backward compatible, I have kept NOOPRetry as the default policy.

guruchai · 2020-02-11T13:54:12Z

...ent/src/main/java/com/hortonworks/registries/schemaregistry/client/SchemaRegistryClient.java

+                throw retryableException;
+            }
+            try {
+                LOG.info("Calling the body : " + targets.rootTarget);


Make it debug

guruchai · 2020-02-11T13:56:25Z

...ent/src/main/java/com/hortonworks/registries/schemaregistry/client/SchemaRegistryClient.java

+                return retryBody.run(targets);
+            } catch (RegistryRetryableException e) {
+                urlSelector.urlWithError(targets.rootTarget.getUri().toString(), e);
+                retryableException = e;


What's the point of making it with error, if we are retry it?

kamalcph

Overall LGTM. Left some comments to address.

kamalcph · 2020-02-12T05:37:45Z

...ent/src/main/java/com/hortonworks/registries/schemaregistry/client/SchemaRegistryClient.java

-                                                                                                  .name())).longValue(),
-                                                         TimeUnit.SECONDS)
-                                      .build();
+                .maximumSize(((Number) configuration.getValue(Configuration.SCHEMA_TEXT_CACHE_SIZE


It's best practice to avoid other changes when fixing the bugs. It helps in cherry-pick the fix to other branches and to do quick code-review.

Yeah I understand, sorry about that.

kamalcph · 2020-02-12T05:38:30Z

...ent/src/main/java/com/hortonworks/registries/schemaregistry/client/SchemaRegistryClient.java

+        ClassLoader classLoader = this.getClass().getClassLoader();
+        Class<? extends RetryPolicy> clazz = null;
+        try {
+            clazz = (Class<? extends RetryPolicy>) Class.forName(retryPolicyClass, true, classLoader);


Why do you need the classLoader? Do you expect the user to load their own RetryPolicy?

I am not quite sure. At the time when I plugged this in, it made sense, now I am not quite sure. Do you guys think this could be a real requirement? @guruchai

Not necessary. It picks current class's classloader anyway.

kamalcph · 2020-02-12T05:57:27Z

...ent/src/main/java/com/hortonworks/registries/schemaregistry/client/SchemaRegistryClient.java

+        try {
+            clazz = (Class<? extends RetryPolicy>) Class.forName(retryPolicyClass, true, classLoader);
+        } catch (ClassNotFoundException e) {
+            throw new RuntimeException("Unable to  Class : " + retryPolicyClass, e);


nit: Unable to initiate the class:

kamalcph · 2020-02-12T05:58:16Z

...ent/src/main/java/com/hortonworks/registries/schemaregistry/client/SchemaRegistryClient.java

-                           .keyManagerFactoryAlgorithm(sslConfigurations.get("keyManagerFactoryAlgorithm"))
-                           .keyManagerFactoryProvider(sslConfigurations.get("keyManagerFactoryProvider"));
+                    .keyStoreFile(sslConfigurations.get(SSL_KEY_STORE_PATH))
+                    .keyStorePassword(sslConfigurations.get("keyStorePassword"))


revert unnecessary changes.

kamalcph · 2020-02-12T06:00:45Z

...istry/common/src/main/java/com/hortonworks/registries/schemaregistry/retry/RetryManager.java

+            }
+        } while (policy.mayBeSleep(++iteration, System.currentTimeMillis() - startTime));
+
+        LOG.debug("Reached the limit of retries for the request after iteration : " +


nit: Use {} instead of string concatenation for readability.

kamalcph · 2020-02-12T06:30:41Z

...istry/common/src/main/java/com/hortonworks/registries/schemaregistry/retry/RetryContext.java

+
+import java.util.Objects;
+
+public class RetryContext<T> {


Create a private constructor to make sure that the instance can only be created through the builder.

kamalcph · 2020-02-12T06:31:39Z

...istry/common/src/main/java/com/hortonworks/registries/schemaregistry/retry/RetryContext.java

+
+public class RetryContext<T> {
+
+    private RetryPolicy policy = new NOOPRetryPolicy();


The builder should set the defaults in the build method.

kamalcph · 2020-02-12T06:32:36Z

...istry/common/src/main/java/com/hortonworks/registries/schemaregistry/retry/RetryContext.java

+
+    public static class Builder<T> {
+
+        private RetryContext<T> retryContext = new RetryContext<T>();


One can reuse the same Builder instance in multiple places. Better to create the RetryContext inside the build method.

Yep, makes sense.

kamalcph · 2020-02-12T06:44:31Z

...ent/src/main/java/com/hortonworks/registries/schemaregistry/client/SchemaRegistryClient.java

-        } catch (LoginException e) {
-            throw new RegistryRetryableException(e);
-        }
+        Response response = RETRY_MANAGER.execute(new RetryContext.Builder<Response>().policy(retryPolicy)


Building the RetryContext with same policy and retryOnException for all the requests is redundant. Can you extract it in a method?

(eg)

@Override public Collection<SchemaVersionKey> findSchemasByFields(SchemaFieldQuery schemaFieldQuery) { return invoke((SchemaRegistryTargets targets) -> { WebTarget target = targets.searchFieldsTarget; for (Map.Entry<String, String> entry : schemaFieldQuery.toQueryMap().entrySet()) { target = target.queryParam(entry.getKey(), entry.getValue()); } return getEntities(target, SchemaVersionKey.class); }); } private <T> T invoke(RetryBody<T> retryBody) { return RETRY_MANAGER.execute(new RetryContext.Builder<T>() .policy(retryPolicy) .retryOnException(RegistryRetryableException.class) .build(() -> runWithFailoverContext(retryBody))); }

I was thinking about removing Request class from RetryContext, then retryManager can be invoked as
retryManager.execute(retryContext, () -> {});
This translates to execute this block code against this retryContext, which makes sense.
That along with your invoke method would clean up the code nicely.

kamalcph · 2020-02-12T06:53:45Z

...ent/src/main/java/com/hortonworks/registries/schemaregistry/client/SchemaRegistryClient.java

+            if (initialWebTarget == null) {
+                initialWebTarget = targets.rootTarget;
+            } else if (initialWebTarget.equals(targets.rootTarget)) {
+                throw retryableException;


Why are you throwing exception here? Please explain it in a comment so that others can understand.

Ok, will add more comments.

This is the part of the code that prompted me in asking you to document how URLSelector and RetryPolicy work together.

kamalcph · 2020-02-12T07:06:21Z

schema-registry/rest-service/src/test/resources/schema-registry-client.yaml

@@ -8,4 +8,11 @@ schema.registry.client.schema.metadata.cache.expiry.interval : 300
 schema.registry.client.schema.text.cache.size : 1024
 schema.registry.client.schema.text.cache.expiry.interval : 300
 schema.registry.client.url.selector : "com.hortonworks.registries.schemaregistry.client.FailoverUrlSelector"


Should the URL selector come under schema.registry.client.retry.strategy config?

We can't move this URL selector config under the retry strategy config, as it will break backward compatibility.

kamalcph · 2020-02-12T07:22:34Z

...common/src/main/java/com/hortonworks/registries/schemaregistry/retry/policy/RetryPolicy.java

+
+    public abstract void init(Map<String, Object> properties);
+
+    public boolean mayBeSleep(int iteration, long timeElapsed) {


please rename the iteration to attempt.

I will rename this to attemptNumber.

kamalcph · 2020-02-12T07:26:41Z

...common/src/main/java/com/hortonworks/registries/schemaregistry/retry/policy/RetryPolicy.java

+        try {
+            Thread.sleep(sleepTimeMs);
+        } catch (InterruptedException e) {
+            throw new RetryPolicyException(e);


Why are we masking the InterruptedException as RetryPolicyException? Can it be handled in RetryManager itself?

Set the interrupt flag if you want to swallow the Interrupted exception and throw it as RuntimeException.
Thread.currentThread().interrupt();

Yeah, this should be RuntimeException instead.

raju-saravanan · 2020-02-14T17:21:27Z

@kamalcph and @guruchai : Addressed PR comments.

raju-saravanan · 2020-02-17T18:14:39Z

@guruchai and @kamalcph: Can you guys review the PR ? I want to merge this by end of day (18 Feb).

kamalcph

Left some minor comments to address.

kamalcph · 2020-02-18T09:15:00Z

.../common/src/test/java/com/hortonworks/registries/schemaregistry/retry/RetryExecutorTest.java

+        });
+    }
+
+    private RetryExecutor createRetryExecutor(long sleepMs, int iteration, long maxSleepMs) {


nit: refactor the variable names w.r.t src code.

kamalcph · 2020-02-18T09:15:57Z

.../common/src/test/java/com/hortonworks/registries/schemaregistry/retry/RetryExecutorTest.java

+        Map<String, Object> props = new HashMap<>();
+        switch (retryPolicyType) {
+            case FIXED:
+                props.put(BackoffPolicy.TIMEOUT_MS, maxSleepMs);


nit: props.put can be placed outside of switch statement.

kamalcph · 2020-02-18T09:19:24Z

.../common/src/test/java/com/hortonworks/registries/schemaregistry/retry/RetryExecutorTest.java

+            }
+        }));
+
+        Assert.assertEquals(2, iteration.get());


maxAttempts is set to 3. Why the expected value in assert statement is 2? (change the iteration to attempt and initialise it to 1).

kamalcph · 2020-02-18T09:25:58Z

.../common/src/test/java/com/hortonworks/registries/schemaregistry/retry/RetryExecutorTest.java

+            }
+        });
+
+        Assert.assertTrue((stopTime.get() - startTime.get() < 2000) && iteration.get() > 1);


This test should assert that the retry gets halted when the #attempts exceed the timeout ((stop - start > timeout)). But, the above statement assert for something else. Please refactor this test case.

nit: Use java.time.Instant for timing operations.

We can remove this test, I don't see any point to it.

kamalcph · 2020-02-18T09:41:22Z

.../common/src/test/java/com/hortonworks/registries/schemaregistry/retry/RetryExecutorTest.java

+    }
+
+    @Test(expected = RuntimeException.class)
+    public void testExceptionMaxIteration() {


Give a meaningful name to the test. (eg) testThrowExceptionOnExceedingMaxAttempts.

kamalcph · 2020-02-18T09:41:32Z

.../common/src/test/java/com/hortonworks/registries/schemaregistry/retry/RetryExecutorTest.java

+    }
+
+    @Test(expected = RuntimeException.class)
+    public void testExceptionForMaxSleep() {


same as above.

kamalcph · 2020-02-18T09:42:03Z

.../common/src/test/java/com/hortonworks/registries/schemaregistry/retry/RetryExecutorTest.java

+    @Test(expected = RuntimeException.class)
+    public void testExceptionForMaxSleep() {
+        createRetryExecutor(100, 1000, 300).execute(() -> {
+            throw new RuntimeException();


Also, test for the state of interrupt flag.

I will come back for more tests in another PR as we are pressed on time.

kamalcph · 2020-02-18T09:58:42Z

...ava/com/hortonworks/registries/schemaregistry/retry/policy/ExponentialBackoffPolicyTest.java

+        float exponent = 2;
+        ExponentialBackoffPolicy exponentialBackoffRetryPolicy = new ExponentialBackoffPolicy(sleepMs, exponent, 10, 10000L);
+        for (int i = 1; i <= 10; i++) {
+            long sleep = (long) (sleepMs * Math.pow(exponent, (i - 1)));


repeating the src formula isn't a good approach. If the formula has error, the test case won't detect it. Expected output for a given input would be good.

kamalcph · 2020-02-18T10:51:54Z

...in/java/com/hortonworks/registries/schemaregistry/retry/policy/ExponentialBackoffPolicy.java

+
+    }
+
+    public ExponentialBackoffPolicy(Long sleepTimeMs, Float exponent, Integer maxAttempts, Long maxSleepTimeMs) {


exponent parameter is unused.

raju-saravanan · 2020-02-19T05:58:15Z

@guruchai: Can I go ahead and merge the PR ?

* Back up * Back up tests * Back up * Revert schema registry changes * Plugin retry manager code in schema registry client * Plug in failover mechanism with retry in schema registry client * Remove retry manager exception from the code * Add support for retry based on exceptions * Fix all APIs to remove calls to currentSchemaRegistryTargets * Address PR comments * Refactor tests

raju-saravanan added 9 commits February 6, 2020 12:30

Back up

561712c

Back up tests

24babbb

Back up

af7627f

Revert schema registry changes

b80de0f

Plugin retry manager code in schema registry client

697b8fc

Plug in failover mechanism with retry in schema registry client

9a380d0

Remove retry manager exception from the code

c8d147a

Add support for retry based on exceptions

bc26980

Fix all APIs to remove calls to currentSchemaRegistryTargets

1526d1a

raju-saravanan requested a review from guruchai February 6, 2020 14:51

guruchai reviewed Feb 11, 2020

View reviewed changes

guruchai requested a review from kamalcph February 11, 2020 14:06

kamalcph suggested changes Feb 12, 2020

View reviewed changes

kamalcph reviewed Feb 12, 2020

View reviewed changes

Address PR comments

13a992c

kamalcph approved these changes Feb 18, 2020

View reviewed changes

kamalcph reviewed Feb 18, 2020

View reviewed changes

Refactor tests

fb80240

raju-saravanan merged commit ce7be7e into master Feb 19, 2020


		import java.util.Map;

		public class ExponentialBackoffRetryPolicy extends RetryPolicy {


		public class NOOPRetryPolicy extends RetryPolicy {

		public NOOPRetryPolicy() {


		public class RetryContext<T> {

		private RetryPolicy policy = new NOOPRetryPolicy();


		public static class Builder<T> {

		private RetryContext<T> retryContext = new RetryContext<T>();


		public abstract void init(Map<String, Object> properties);

		public boolean mayBeSleep(int iteration, long timeElapsed) {


		}

		public ExponentialBackoffPolicy(Long sleepTimeMs, Float exponent, Integer maxAttempts, Long maxSleepTimeMs) {

ISSUE-640 : Fix URL failover mechanism in Schema Registry Client #664

ISSUE-640 : Fix URL failover mechanism in Schema Registry Client #664

Conversation

raju-saravanan commented Feb 6, 2020 • edited Loading

guruchai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamalcph Feb 12, 2020 • 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

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

kamalcph 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

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

kamalcph Feb 12, 2020 • 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

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

raju-saravanan Feb 13, 2020 • 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

raju-saravanan commented Feb 14, 2020

raju-saravanan commented Feb 17, 2020

kamalcph 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

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

raju-saravanan commented Feb 19, 2020

raju-saravanan commented Feb 6, 2020 •

edited

Loading

kamalcph Feb 12, 2020 •

edited

Loading

kamalcph Feb 12, 2020 •

edited

Loading

raju-saravanan Feb 13, 2020 •

edited

Loading