Hi Amareshwari, https://reviews.apache.org/r/64946
This is the review request raised quite long back and reviewed by Rajat. I have updated on the jira just today. Regards Rajitha On Tue, Jul 10, 2018 at 1:18 PM amareshwarisr <amareshw...@gmail.com> wrote: > Rajitha, > > Seeing review request and commit at the same time. Was this by mistake? > > Thanks > > On Tue, Jul 10, 2018 at 12:55 PM, <rajit...@apache.org> wrote: > >> Repository: lens >> Updated Branches: >> refs/heads/master 278a02473 -> 4a24fb94f >> >> >> LENS-1371 : Add a substring based retry policy for failures >> >> >> Project: http://git-wip-us.apache.org/repos/asf/lens/repo >> Commit: http://git-wip-us.apache.org/repos/asf/lens/commit/4a24fb94 >> Tree: http://git-wip-us.apache.org/repos/asf/lens/tree/4a24fb94 >> Diff: http://git-wip-us.apache.org/repos/asf/lens/diff/4a24fb94 >> >> Branch: refs/heads/master >> Commit: 4a24fb94f81554f6de2eec5f0cc86fe4e80c1ac9 >> Parents: 278a024 >> Author: Rajitha R <rajit...@apache.org> >> Authored: Tue Jul 10 12:55:20 2018 +0530 >> Committer: Rajitha.R <rajit...@im0318-l0.corp.inmobi.com> >> Committed: Tue Jul 10 12:55:20 2018 +0530 >> >> ---------------------------------------------------------------------- >> .../apache/lens/cube/parse/TestVirtualFactQueries.java | 8 ++++---- >> .../src/main/resources/hivedriver-default.xml | 12 ++++++++++++ >> .../java/org/apache/lens/driver/jdbc/JDBCDriver.java | 9 +++++++-- >> .../src/main/resources/jdbcdriver-default.xml | 12 ++++++++++++ >> .../org/apache/lens/driver/jdbc/TestJdbcDriver.java | 8 -------- >> .../org/apache/lens/server/api/LensConfConstants.java | 5 +++++ >> .../lens/server/api/retry/BackOffRetryHandler.java | 5 ++++- >> .../retry/FibonacciExponentialBackOffRetryHandler.java | 4 ++++ >> .../lens/server/api/retry/ImmediateRetryHandler.java | 6 +++++- >> .../apache/lens/server/api/retry/NoRetryHandler.java | 5 +++++ >> .../lens/server/query/QueryExecutionServiceImpl.java | 11 +++++++++-- >> src/site/apt/admin/hivedriver-config.apt | 4 ++++ >> src/site/apt/admin/jdbcdriver-config.apt | 4 ++++ >> 13 files changed, 75 insertions(+), 18 deletions(-) >> ---------------------------------------------------------------------- >> >> >> >> http://git-wip-us.apache.org/repos/asf/lens/blob/4a24fb94/lens-cube/src/test/java/org/apache/lens/cube/parse/TestVirtualFactQueries.java >> ---------------------------------------------------------------------- >> diff --git >> a/lens-cube/src/test/java/org/apache/lens/cube/parse/TestVirtualFactQueries.java >> b/lens-cube/src/test/java/org/apache/lens/cube/parse/TestVirtualFactQueries.java >> index e2f1074..81a8972 100644 >> --- >> a/lens-cube/src/test/java/org/apache/lens/cube/parse/TestVirtualFactQueries.java >> +++ >> b/lens-cube/src/test/java/org/apache/lens/cube/parse/TestVirtualFactQueries.java >> @@ -71,10 +71,10 @@ public class TestVirtualFactQueries extends >> TestQueryRewrite { >> @Test >> public void testVirtualFactColStartTimeQuery() { >> try { >> - rewriteCtx("select dim1,SUM(msr1) from virtualcube where " + >> TWO_DAYS_RANGE, getConfWithStorages("C1")); >> - Assert.fail("Should not come here..Column Start time feature is >> failing.."); >> + rewriteCtx("select dim1,SUM(msr1) from virtualcube where " + >> TWO_DAYS_RANGE, getConfWithStorages("C1")); >> + Assert.fail("Should not come here..Column Start time feature is >> failing.."); >> }catch (LensException e) { >> - if(e.getErrorCode() == 3024) { >> + if (e.getErrorCode() == 3024) { >> System.out.println("Expected flow :" + e.getMessage()); >> }else { >> Assert.fail("Exception not as expected"); >> @@ -88,7 +88,7 @@ public class TestVirtualFactQueries extends >> TestQueryRewrite { >> rewriteCtx("select dim2,SUM(msr1) from virtualcube where " + >> TWO_DAYS_RANGE, getConfWithStorages("C1")); >> Assert.fail("Should not come here..Column End time feature is >> failing.."); >> }catch (LensException e) { >> - if(e.getErrorCode() == 3024) { >> + if (e.getErrorCode() == 3024) { >> System.out.println("Expected flow :" + e.getMessage()); >> }else { >> Assert.fail("Exception not as expected"); >> >> >> http://git-wip-us.apache.org/repos/asf/lens/blob/4a24fb94/lens-driver-hive/src/main/resources/hivedriver-default.xml >> ---------------------------------------------------------------------- >> diff --git a/lens-driver-hive/src/main/resources/hivedriver-default.xml >> b/lens-driver-hive/src/main/resources/hivedriver-default.xml >> index d41e36a..2776123 100644 >> --- a/lens-driver-hive/src/main/resources/hivedriver-default.xml >> +++ b/lens-driver-hive/src/main/resources/hivedriver-default.xml >> @@ -185,4 +185,16 @@ >> <description>Cost based Query type mapping</description> >> </property> >> >> + <property> >> + <name>retry.messages.contains.map</name> >> + <value>Session handle not >> found=org.apache.lens.server.api.retry.ImmediateRetryHandler(2)</value> >> + <description>Comma separated error messages and retry >> policy</description> >> + </property> >> + >> + <property> >> + <name>query.retry.policy.classes</name> >> + >> <value>org.apache.lens.server.api.retry.SubstringMessagePolicyDecider</value> >> + <description>List of policy decider classes</description> >> + </property> >> + >> </configuration> >> >> >> http://git-wip-us.apache.org/repos/asf/lens/blob/4a24fb94/lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java >> ---------------------------------------------------------------------- >> diff --git >> a/lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java >> b/lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java >> index e810e7c..8dd299a 100644 >> --- >> a/lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java >> +++ >> b/lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java >> @@ -278,6 +278,7 @@ public class JDBCDriver extends AbstractLensDriver { >> try { >> stmt = createStatement(conn); >> result.stmt = stmt; >> + >> Boolean isResultAvailable = >> stmt.execute(queryContext.getRewrittenQuery()); >> if >> (queryContext.getLensContext().getDriverStatus().isCanceled()) { >> return result; >> @@ -992,8 +993,12 @@ public class JDBCDriver extends AbstractLensDriver { >> checkConfigured(); >> try { >> JdbcQueryContext ctx = getQueryContext(handle); >> - ctx.getResultFuture().cancel(true); >> - ctx.closeResult(); >> + if (ctx != null) { >> + ctx.getResultFuture().cancel(true); >> + ctx.closeResult(); >> + } >> + } catch (LensException exc) { >> + log.error("{} Failed to close query {}", getFullyQualifiedName(), >> handle.getHandleId()); >> } finally { >> queryContextMap.remove(handle); >> } >> >> >> http://git-wip-us.apache.org/repos/asf/lens/blob/4a24fb94/lens-driver-jdbc/src/main/resources/jdbcdriver-default.xml >> ---------------------------------------------------------------------- >> diff --git a/lens-driver-jdbc/src/main/resources/jdbcdriver-default.xml >> b/lens-driver-jdbc/src/main/resources/jdbcdriver-default.xml >> index 511fe71..2c69eef 100644 >> --- a/lens-driver-jdbc/src/main/resources/jdbcdriver-default.xml >> +++ b/lens-driver-jdbc/src/main/resources/jdbcdriver-default.xml >> @@ -294,4 +294,16 @@ >> <description>Cost based Query type mapping</description> >> </property> >> >> + <property> >> + <name>retry.messages.contains.map</name> >> + <value>Query not >> found=org.apache.lens.server.api.retry.ImmediateRetryHandler(2)</value> >> + <description>Comma separated error messages and retry >> policy</description> >> + </property> >> + >> + <property> >> + <name>query.retry.policy.classes</name> >> + >> <value>org.apache.lens.server.api.retry.SubstringMessagePolicyDecider</value> >> + <description>List of classes to decide policies</description> >> + </property> >> + >> </configuration> >> >> >> http://git-wip-us.apache.org/repos/asf/lens/blob/4a24fb94/lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java >> ---------------------------------------------------------------------- >> diff --git >> a/lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java >> b/lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java >> index 506935b..c08b04a 100644 >> --- >> a/lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java >> +++ >> b/lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java >> @@ -768,14 +768,6 @@ public class TestJdbcDriver { >> } >> >> driver.closeQuery(handle); >> - // Close again, should get not found >> - try { >> - driver.closeQuery(handle); >> - fail("Close again should have thrown exception"); >> - } catch (LensException ex) { >> - assertTrue(ex.getMessage().contains("not found") && >> ex.getMessage().contains(handle.getHandleId().toString())); >> - System.out.println("Matched exception"); >> - } >> } else { >> fail("Only in memory result set is supported as of now"); >> } >> >> >> http://git-wip-us.apache.org/repos/asf/lens/blob/4a24fb94/lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java >> ---------------------------------------------------------------------- >> diff --git >> a/lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java >> b/lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java >> index 2147f08..58a235a 100644 >> --- >> a/lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java >> +++ >> b/lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java >> @@ -1341,4 +1341,9 @@ public final class LensConfConstants { >> >> public static final String SSL_KEYSTORE_PASSWORD = SERVER_PFX + >> "ssl.password"; >> >> + /** >> + * Message map for configured policy >> + */ >> + public static final String RETRY_MESSAGE_MAP = >> "retry.messages.contains.map"; >> + >> } >> >> >> http://git-wip-us.apache.org/repos/asf/lens/blob/4a24fb94/lens-server-api/src/main/java/org/apache/lens/server/api/retry/BackOffRetryHandler.java >> ---------------------------------------------------------------------- >> diff --git >> a/lens-server-api/src/main/java/org/apache/lens/server/api/retry/BackOffRetryHandler.java >> b/lens-server-api/src/main/java/org/apache/lens/server/api/retry/BackOffRetryHandler.java >> index 5ea5710..2d8b8e4 100644 >> --- >> a/lens-server-api/src/main/java/org/apache/lens/server/api/retry/BackOffRetryHandler.java >> +++ >> b/lens-server-api/src/main/java/org/apache/lens/server/api/retry/BackOffRetryHandler.java >> @@ -41,7 +41,10 @@ import java.io.Serializable; >> * } >> * } >> * >> - * Note that this is only one of the possible use cases, other complex >> use cases are in retry framework. >> + * * Note >> + * 1. This is only one of the possible use cases, other complex use >> cases are in retry framework. >> + * 2. Every implementation needs an all String arguments constructor >> which can be used in >> + * {@link SubstringMessagePolicyDecider} >> */ >> public interface BackOffRetryHandler<FC extends FailureContext> extends >> Serializable { >> >> >> >> http://git-wip-us.apache.org/repos/asf/lens/blob/4a24fb94/lens-server-api/src/main/java/org/apache/lens/server/api/retry/FibonacciExponentialBackOffRetryHandler.java >> ---------------------------------------------------------------------- >> diff --git >> a/lens-server-api/src/main/java/org/apache/lens/server/api/retry/FibonacciExponentialBackOffRetryHandler.java >> b/lens-server-api/src/main/java/org/apache/lens/server/api/retry/FibonacciExponentialBackOffRetryHandler.java >> index 01da25d..be3fc52 100644 >> --- >> a/lens-server-api/src/main/java/org/apache/lens/server/api/retry/FibonacciExponentialBackOffRetryHandler.java >> +++ >> b/lens-server-api/src/main/java/org/apache/lens/server/api/retry/FibonacciExponentialBackOffRetryHandler.java >> @@ -33,6 +33,10 @@ public class >> FibonacciExponentialBackOffRetryHandler<FC extends FailureContext> >> final long maxDelay; >> final long waitMillis; >> >> + public FibonacciExponentialBackOffRetryHandler(String numRetries, >> String maxDelay, String waitMillis) { >> + this(Integer.parseInt(numRetries), Long.parseLong(maxDelay), >> Long.parseLong(waitMillis)); >> + } >> + >> public FibonacciExponentialBackOffRetryHandler(int numRetries, long >> maxDelay, long waitMillis) { >> checkArgument(numRetries > 2); >> fibonacci = new int[numRetries]; >> >> >> http://git-wip-us.apache.org/repos/asf/lens/blob/4a24fb94/lens-server-api/src/main/java/org/apache/lens/server/api/retry/ImmediateRetryHandler.java >> ---------------------------------------------------------------------- >> diff --git >> a/lens-server-api/src/main/java/org/apache/lens/server/api/retry/ImmediateRetryHandler.java >> b/lens-server-api/src/main/java/org/apache/lens/server/api/retry/ImmediateRetryHandler.java >> index c1c0126..cea9df3 100644 >> --- >> a/lens-server-api/src/main/java/org/apache/lens/server/api/retry/ImmediateRetryHandler.java >> +++ >> b/lens-server-api/src/main/java/org/apache/lens/server/api/retry/ImmediateRetryHandler.java >> @@ -29,6 +29,10 @@ public class ImmediateRetryHandler<FC extends >> FailureContext> implements BackOff >> this(1); >> } >> >> + public ImmediateRetryHandler(String retries) { >> + this(Integer.parseInt(retries)); >> + } >> + >> @Override >> public boolean canTryOpNow(FC failContext) { >> return true; >> @@ -41,6 +45,6 @@ public class ImmediateRetryHandler<FC extends >> FailureContext> implements BackOff >> >> @Override >> public boolean hasExhaustedRetries(FC failContext) { >> - return ++retriesDone > retries; >> + return failContext.getFailCount() > retries; >> } >> } >> >> >> http://git-wip-us.apache.org/repos/asf/lens/blob/4a24fb94/lens-server-api/src/main/java/org/apache/lens/server/api/retry/NoRetryHandler.java >> ---------------------------------------------------------------------- >> diff --git >> a/lens-server-api/src/main/java/org/apache/lens/server/api/retry/NoRetryHandler.java >> b/lens-server-api/src/main/java/org/apache/lens/server/api/retry/NoRetryHandler.java >> index df68a48..0a2dac1 100644 >> --- >> a/lens-server-api/src/main/java/org/apache/lens/server/api/retry/NoRetryHandler.java >> +++ >> b/lens-server-api/src/main/java/org/apache/lens/server/api/retry/NoRetryHandler.java >> @@ -32,4 +32,9 @@ public class NoRetryHandler<FC extends FailureContext> >> extends ImmediateRetryHan >> public long getOperationNextTime(FC failContext) { >> return Long.MAX_VALUE; >> } >> + >> + @Override >> + public boolean hasExhaustedRetries(FC failContext) { >> + return true; >> + } >> } >> >> >> http://git-wip-us.apache.org/repos/asf/lens/blob/4a24fb94/lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java >> ---------------------------------------------------------------------- >> diff --git >> a/lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java >> b/lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java >> index 3f1731e..4023a24 100644 >> --- >> a/lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java >> +++ >> b/lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java >> @@ -47,6 +47,7 @@ import org.apache.lens.api.query.*; >> import org.apache.lens.api.query.QueryStatus.Status; >> import org.apache.lens.cube.metadata.DateUtil; >> import org.apache.lens.driver.hive.HiveDriver; >> +import org.apache.lens.driver.jdbc.JDBCDriver; >> import org.apache.lens.server.BaseLensService; >> import org.apache.lens.server.LensServerConf; >> import org.apache.lens.server.LensServices; >> @@ -913,7 +914,11 @@ public class QueryExecutionServiceImpl extends >> BaseLensService implements QueryE >> if (removeFromLaunchedQueries(ctx)) { >> processWaitingQueriesAsync(ctx); >> } >> - if (ctx.getDriverStatus().failed() && >> !getDriverRetryPolicy(ctx).hasExhaustedRetries(ctx)) { >> + >> + /*Upon server restarts, the driver status isn't reliable in case >> of jdbc due to query not found error, >> + hence we fall back to the context status and retry */ >> + if ((ctx.getDriverStatus().failed() || (ctx.getStatus().failing() >> + && ctx.getSelectedDriver() instanceof JDBCDriver)) && >> !getDriverRetryPolicy(ctx).hasExhaustedRetries(ctx)) { >> log.info("query {} will be retried on the same driver {}", >> ctx.getQueryHandle(), >> ctx.getSelectedDriver().getFullyQualifiedName()); >> ctx.extractFailedAttempt(); >> @@ -975,8 +980,10 @@ public class QueryExecutionServiceImpl extends >> BaseLensService implements QueryE >> >> private BackOffRetryHandler<QueryContext> >> getDriverRetryPolicy(QueryContext ctx) { >> if (ctx.getDriverRetryPolicy() == null) { >> + String errorMessage = ctx.getDriverStatus().getErrorMessage() != >> null ? ctx.getDriverStatus().getErrorMessage() >> + : ctx.getStatus().getErrorMessage(); >> >> ctx.setDriverRetryPolicy(ctx.getSelectedDriver().getRetryPolicyDecider() >> - .decidePolicy(ctx.getDriverStatus().getErrorMessage())); >> + .decidePolicy(errorMessage)); >> } >> return ctx.getDriverRetryPolicy(); >> } >> >> >> http://git-wip-us.apache.org/repos/asf/lens/blob/4a24fb94/src/site/apt/admin/hivedriver-config.apt >> ---------------------------------------------------------------------- >> diff --git a/src/site/apt/admin/hivedriver-config.apt >> b/src/site/apt/admin/hivedriver-config.apt >> index 71208bb..5d99892 100644 >> --- a/src/site/apt/admin/hivedriver-config.apt >> +++ b/src/site/apt/admin/hivedriver-config.apt >> @@ -78,4 +78,8 @@ Hive driver configuration >> *--+--+---+--+ >> |20|lens.driver.hive.waiting.queries.selection.policy.factories| >> |Factories used to instantiate driver specific waiting queries selection >> policies. Every factory should be an implementation of >> org.apache.lens.server.api.common.ConfigBasedObjectCreationFactory and >> create an implementation of >> org.apache.lens.server.api.query.collect.WaitingQueriesSelectionPolicy.| >> *--+--+---+--+ >> +|21|query.retry.policy.classes|org.apache.lens.server.api.retry.SubstringMessagePolicyDecider|List >> of policy decider classes| >> +*--+--+---+--+ >> +|22|retry.messages.contains.map|Session handle not >> found=org.apache.lens.server.api.retry.ImmediateRetryHandler(2)|Comma >> separated error messages and retry policy| >> +*--+--+---+--+ >> The configuration parameters and their default values >> >> >> http://git-wip-us.apache.org/repos/asf/lens/blob/4a24fb94/src/site/apt/admin/jdbcdriver-config.apt >> ---------------------------------------------------------------------- >> diff --git a/src/site/apt/admin/jdbcdriver-config.apt >> b/src/site/apt/admin/jdbcdriver-config.apt >> index c50872b..792278e 100644 >> --- a/src/site/apt/admin/jdbcdriver-config.apt >> +++ b/src/site/apt/admin/jdbcdriver-config.apt >> @@ -101,4 +101,8 @@ Jdbc driver configuration >> *--+--+---+--+ >> |38|lens.query.timeout.millis|3600000|The runtime(millis) of the query >> after which query will be timedout and cancelled. Default is 1 hour for >> jdbc queries.| >> *--+--+---+--+ >> +|39|query.retry.policy.classes|org.apache.lens.server.api.retry.SubstringMessagePolicyDecider|List >> of classes to decide policies| >> +*--+--+---+--+ >> +|40|retry.messages.contains.map|Query not >> found=org.apache.lens.server.api.retry.ImmediateRetryHandler(2)|Comma >> separated error messages and retry policy| >> +*--+--+---+--+ >> The configuration parameters and their default values >> >> > -- Regards, Rajitha.R
