[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[ https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17619424#comment-17619424 ] ASF GitHub Bot commented on DRILL-8314: --- jnturton merged PR #2655: URL: https://github.com/apache/drill/pull/2655 > Add support for automatically retrying and disabling broken storage plugins > --- > > Key: DRILL-8314 > URL: https://issues.apache.org/jira/browse/DRILL-8314 > Project: Apache Drill > Issue Type: Improvement > Components: Storage - Other >Affects Versions: 1.20.2 >Reporter: James Turton >Assignee: James Turton >Priority: Minor > Fix For: 2.0.0 > > > Enabled storage plugins may malfunction for different reasons, e.g. they have > been misconfigured or their remote data source has gone offline. Depending on > the plugin's implementation, this could cause it to fail to return optimizer > rules or register schemas. In some cases this can have a wider impact, e.g. > unconditioned queries against the info schema will fail if a single plugin is > failing in registerSchemas. > Rather than us swallowing such errors and silently returning a subset of > results, this Jira proposes new features that allow firstly for the retrying > of attempts to get rules or schemas from a plugin and secondly for a plugin > to be automatically disabled after the configured maximum number of attempts > have failed. A broken plugin will still cause a query failure but the user > will be informed that the plugin has been disabled as a result. Knowing what > has just happened, the user can choose to reissue the query knowing that the > broken plugin is now disabled or they might choose to investigate the problem > affecting the broken plugin instead. > These new features are optional and can be disabled using new SYSTEM options. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[
https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17615280#comment-17615280
]
ASF GitHub Bot commented on DRILL-8314:
---
vvysotskyi commented on code in PR #2655:
URL: https://github.com/apache/drill/pull/2655#discussion_r991589179
##
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java:
##
@@ -34,12 +34,22 @@ public interface StoragePluginRegistry extends
Iterable getDataSource(UserCredentials
userCredentials) {
));
}
- public SqlDialect getDialect(DataSource dataSource) {
-return JdbcSchema.createDialect(
- SqlDialectFactoryImpl.INSTANCE,
- dataSource
-);
+ public synchronized SqlDialect getDialect(UserCredentials userCredentials) {
+if (sqlDialect == null) {
Review Comment:
Please use double-check to avoid extra overhead caused by synchronization
for most of the calls when the `sqlDialect` is already created.
> Add support for automatically retrying and disabling broken storage plugins
> ---
>
> Key: DRILL-8314
> URL: https://issues.apache.org/jira/browse/DRILL-8314
> Project: Apache Drill
> Issue Type: Improvement
> Components: Storage - Other
>Affects Versions: 1.20.2
>Reporter: James Turton
>Assignee: James Turton
>Priority: Minor
> Fix For: 2.0.0
>
>
> Enabled storage plugins may malfunction for different reasons, e.g. they have
> been misconfigured or their remote data source has gone offline. Depending on
> the plugin's implementation, this could cause it to fail to return optimizer
> rules or register schemas. In some cases this can have a wider impact, e.g.
> unconditioned queries against the info schema will fail if a single plugin is
> failing in registerSchemas.
> Rather than us swallowing such errors and silently returning a subset of
> results, this Jira proposes new features that allow firstly for the retrying
> of attempts to get rules or schemas from a plugin and secondly for a plugin
> to be automatically disabled after the configured maximum number of attempts
> have failed. A broken plugin will still cause a query failure but the user
> will be informed that the plugin has been disabled as a result. Knowing what
> has just happened, the user can choose to reissue the query knowing that the
> broken plugin is now disabled or they might choose to investigate the problem
> affecting the broken plugin instead.
> These new features are optional and can be disabled using new SYSTEM options.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[
https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17615077#comment-17615077
]
ASF GitHub Bot commented on DRILL-8314:
---
jnturton commented on code in PR #2655:
URL: https://github.com/apache/drill/pull/2655#discussion_r991218134
##
exec/java-exec/src/main/java/org/apache/calcite/jdbc/DynamicRootSchema.java:
##
@@ -91,22 +91,69 @@ private CalciteSchema getSchema(String schemaName, boolean
caseSensitive) {
public SchemaPath resolveTableAlias(String alias) {
return Optional.ofNullable(aliasRegistryProvider.getTableAliasesRegistry()
-.getUserAliases(schemaConfig.getUserName()).get(alias))
+ .getUserAliases(schemaConfig.getUserName()).get(alias))
.map(SchemaPath::parseFromString)
.orElse(null);
}
+ private void attemptToRegisterSchemas(StoragePlugin plugin) throws Exception
{
+long maxAttempts = schemaConfig
+ .getOption(ExecConstants.STORAGE_PLUGIN_ACCESS_ATTEMPTS)
+ .num_val;
+long attemptDelayMs = schemaConfig
+ .getOption(ExecConstants.STORAGE_PLUGIN_ATTEMPT_DELAY)
+ .num_val;
+int attempt=0;
+Exception lastAttemptEx = null;
+
+while (attempt++ < maxAttempts) {
Review Comment:
@vvysotskyi let me know if you still prefer the use of a library for the
retry logic.
> Add support for automatically retrying and disabling broken storage plugins
> ---
>
> Key: DRILL-8314
> URL: https://issues.apache.org/jira/browse/DRILL-8314
> Project: Apache Drill
> Issue Type: Improvement
> Components: Storage - Other
>Affects Versions: 1.20.2
>Reporter: James Turton
>Assignee: James Turton
>Priority: Minor
> Fix For: 2.0.0
>
>
> Enabled storage plugins may malfunction for different reasons, e.g. they have
> been misconfigured or their remote data source has gone offline. Depending on
> the plugin's implementation, this could cause it to fail to return optimizer
> rules or register schemas. In some cases this can have a wider impact, e.g.
> unconditioned queries against the info schema will fail if a single plugin is
> failing in registerSchemas.
> Rather than us swallowing such errors and silently returning a subset of
> results, this Jira proposes new features that allow firstly for the retrying
> of attempts to get rules or schemas from a plugin and secondly for a plugin
> to be automatically disabled after the configured maximum number of attempts
> have failed. A broken plugin will still cause a query failure but the user
> will be informed that the plugin has been disabled as a result. Knowing what
> has just happened, the user can choose to reissue the query knowing that the
> broken plugin is now disabled or they might choose to investigate the problem
> affecting the broken plugin instead.
> These new features are optional and can be disabled using new SYSTEM options.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[
https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17610345#comment-17610345
]
ASF GitHub Bot commented on DRILL-8314:
---
jnturton commented on code in PR #2655:
URL: https://github.com/apache/drill/pull/2655#discussion_r981927883
##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java:
##
@@ -130,11 +132,18 @@ private static PhysicalPlan convertPlan(QueryContext
context, String sql, Pointe
logger.trace("There was an error during conversion into physical plan. "
+
"Will sync remote and local function registries if needed and retry
" +
"in case if issue was due to missing function implementation.", e);
- // it is prohibited to retry query planning for ANALYZE statement since
it changes
- // query-level option values and will fail when rerunning with updated
values
- if (context.getFunctionRegistry().syncWithRemoteRegistry(
- context.getDrillOperatorTable().getFunctionRegistryVersion())
-&& context.getSQLStatementType() != SqlStatementType.ANALYZE) {
+
+ int funcRegVer =
context.getDrillOperatorTable().getFunctionRegistryVersion();
+ // We do not retry conversion if the error is a UserException of type
RESOURCE
+ boolean isResourceErr = e instanceof UserException && ((UserException)
e).getErrorType() == RESOURCE;
Review Comment:
@vvysotskyi do you think that some new variable in UserException or
UserExceptionContext which indicates that a plugin was disabled as a result of
the underlying error would be preferable? This exception is visible in all of
the needed places and seems like a pretty natural home for this information...
> Add support for automatically retrying and disabling broken storage plugins
> ---
>
> Key: DRILL-8314
> URL: https://issues.apache.org/jira/browse/DRILL-8314
> Project: Apache Drill
> Issue Type: Improvement
> Components: Storage - Other
>Affects Versions: 1.20.2
>Reporter: James Turton
>Assignee: James Turton
>Priority: Minor
> Fix For: 2.0.0
>
>
> Enabled storage plugins may malfunction for different reasons, e.g. they have
> been misconfigured or their remote data source has gone offline. Depending on
> the plugin's implementation, this could cause it to fail to return optimizer
> rules or register schemas. In some cases this can have a wider impact, e.g.
> unconditioned queries against the info schema will fail if a single plugin is
> failing in registerSchemas.
> Rather than us swallowing such errors and silently returning a subset of
> results, this Jira proposes new features that allow firstly for the retrying
> of attempts to get rules or schemas from a plugin and secondly for a plugin
> to be automatically disabled after the configured maximum number of attempts
> have failed. A broken plugin will still cause a query failure but the user
> will be informed that the plugin has been disabled as a result. Knowing what
> has just happened, the user can choose to reissue the query knowing that the
> broken plugin is now disabled or they might choose to investigate the problem
> affecting the broken plugin instead.
> These new features are optional and can be disabled using new SYSTEM options.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[
https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17610244#comment-17610244
]
ASF GitHub Bot commented on DRILL-8314:
---
vvysotskyi commented on code in PR #2655:
URL: https://github.com/apache/drill/pull/2655#discussion_r981716643
##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SqlHandlerConfig.java:
##
@@ -52,12 +59,81 @@ public QueryContext getContext() {
return context;
}
+ private RuleSet attemptToGetRules(PlannerPhase phase,
Collection plugins) throws PluginException{
Review Comment:
I meant to drop both retry and auto-disabling logic. I have checked JDBC
plugin, and it has several places where the data source is created. One of them
is `getOptimizerRules` which is used here, and another one is
`registerSchemas`, which is called during validation, before this place. So if
we have something broken, it should fail before calling this method.
> Add support for automatically retrying and disabling broken storage plugins
> ---
>
> Key: DRILL-8314
> URL: https://issues.apache.org/jira/browse/DRILL-8314
> Project: Apache Drill
> Issue Type: Improvement
> Components: Storage - Other
>Affects Versions: 1.20.2
>Reporter: James Turton
>Assignee: James Turton
>Priority: Minor
> Fix For: 2.0.0
>
>
> Enabled storage plugins may malfunction for different reasons, e.g. they have
> been misconfigured or their remote data source has gone offline. Depending on
> the plugin's implementation, this could cause it to fail to return optimizer
> rules or register schemas. In some cases this can have a wider impact, e.g.
> unconditioned queries against the info schema will fail if a single plugin is
> failing in registerSchemas.
> Rather than us swallowing such errors and silently returning a subset of
> results, this Jira proposes new features that allow firstly for the retrying
> of attempts to get rules or schemas from a plugin and secondly for a plugin
> to be automatically disabled after the configured maximum number of attempts
> have failed. A broken plugin will still cause a query failure but the user
> will be informed that the plugin has been disabled as a result. Knowing what
> has just happened, the user can choose to reissue the query knowing that the
> broken plugin is now disabled or they might choose to investigate the problem
> affecting the broken plugin instead.
> These new features are optional and can be disabled using new SYSTEM options.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[
https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17610238#comment-17610238
]
ASF GitHub Bot commented on DRILL-8314:
---
vvysotskyi commented on code in PR #2655:
URL: https://github.com/apache/drill/pull/2655#discussion_r981705289
##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java:
##
@@ -130,11 +132,18 @@ private static PhysicalPlan convertPlan(QueryContext
context, String sql, Pointe
logger.trace("There was an error during conversion into physical plan. "
+
"Will sync remote and local function registries if needed and retry
" +
"in case if issue was due to missing function implementation.", e);
- // it is prohibited to retry query planning for ANALYZE statement since
it changes
- // query-level option values and will fail when rerunning with updated
values
- if (context.getFunctionRegistry().syncWithRemoteRegistry(
- context.getDrillOperatorTable().getFunctionRegistryVersion())
-&& context.getSQLStatementType() != SqlStatementType.ANALYZE) {
+
+ int funcRegVer =
context.getDrillOperatorTable().getFunctionRegistryVersion();
+ // We do not retry conversion if the error is a UserException of type
RESOURCE
+ boolean isResourceErr = e instanceof UserException && ((UserException)
e).getErrorType() == RESOURCE;
Review Comment:
Yes, it would be a more reliable approach. Initially, I was looking for a
way how to pass it properly, one of the options is to use the query option, but
it will be tricky to set it in the place where plugins could be disabled with
this feature.
> Add support for automatically retrying and disabling broken storage plugins
> ---
>
> Key: DRILL-8314
> URL: https://issues.apache.org/jira/browse/DRILL-8314
> Project: Apache Drill
> Issue Type: Improvement
> Components: Storage - Other
>Affects Versions: 1.20.2
>Reporter: James Turton
>Assignee: James Turton
>Priority: Minor
> Fix For: 2.0.0
>
>
> Enabled storage plugins may malfunction for different reasons, e.g. they have
> been misconfigured or their remote data source has gone offline. Depending on
> the plugin's implementation, this could cause it to fail to return optimizer
> rules or register schemas. In some cases this can have a wider impact, e.g.
> unconditioned queries against the info schema will fail if a single plugin is
> failing in registerSchemas.
> Rather than us swallowing such errors and silently returning a subset of
> results, this Jira proposes new features that allow firstly for the retrying
> of attempts to get rules or schemas from a plugin and secondly for a plugin
> to be automatically disabled after the configured maximum number of attempts
> have failed. A broken plugin will still cause a query failure but the user
> will be informed that the plugin has been disabled as a result. Knowing what
> has just happened, the user can choose to reissue the query knowing that the
> broken plugin is now disabled or they might choose to investigate the problem
> affecting the broken plugin instead.
> These new features are optional and can be disabled using new SYSTEM options.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[
https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17610094#comment-17610094
]
ASF GitHub Bot commented on DRILL-8314:
---
jnturton commented on code in PR #2655:
URL: https://github.com/apache/drill/pull/2655#discussion_r981328928
##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java:
##
@@ -130,11 +132,18 @@ private static PhysicalPlan convertPlan(QueryContext
context, String sql, Pointe
logger.trace("There was an error during conversion into physical plan. "
+
"Will sync remote and local function registries if needed and retry
" +
"in case if issue was due to missing function implementation.", e);
- // it is prohibited to retry query planning for ANALYZE statement since
it changes
- // query-level option values and will fail when rerunning with updated
values
- if (context.getFunctionRegistry().syncWithRemoteRegistry(
- context.getDrillOperatorTable().getFunctionRegistryVersion())
-&& context.getSQLStatementType() != SqlStatementType.ANALYZE) {
+
+ int funcRegVer =
context.getDrillOperatorTable().getFunctionRegistryVersion();
+ // We do not retry conversion if the error is a UserException of type
RESOURCE
+ boolean isResourceErr = e instanceof UserException && ((UserException)
e).getErrorType() == RESOURCE;
Review Comment:
Maybe after plugin auto disabling has happened I should set a flag somewhere
like QueryContext to record that it has happened. Then this code doesn't need
to try to tell what happened from the exception type, it can look at the flag
and see that it should not try to sync the function registry and convert the
query again because a needed plugin was broken and is now disabled?
> Add support for automatically retrying and disabling broken storage plugins
> ---
>
> Key: DRILL-8314
> URL: https://issues.apache.org/jira/browse/DRILL-8314
> Project: Apache Drill
> Issue Type: Improvement
> Components: Storage - Other
>Affects Versions: 1.20.2
>Reporter: James Turton
>Assignee: James Turton
>Priority: Minor
> Fix For: 2.0.0
>
>
> Enabled storage plugins may malfunction for different reasons, e.g. they have
> been misconfigured or their remote data source has gone offline. Depending on
> the plugin's implementation, this could cause it to fail to return optimizer
> rules or register schemas. In some cases this can have a wider impact, e.g.
> unconditioned queries against the info schema will fail if a single plugin is
> failing in registerSchemas.
> Rather than us swallowing such errors and silently returning a subset of
> results, this Jira proposes new features that allow firstly for the retrying
> of attempts to get rules or schemas from a plugin and secondly for a plugin
> to be automatically disabled after the configured maximum number of attempts
> have failed. A broken plugin will still cause a query failure but the user
> will be informed that the plugin has been disabled as a result. Knowing what
> has just happened, the user can choose to reissue the query knowing that the
> broken plugin is now disabled or they might choose to investigate the problem
> affecting the broken plugin instead.
> These new features are optional and can be disabled using new SYSTEM options.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[
https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17609599#comment-17609599
]
ASF GitHub Bot commented on DRILL-8314:
---
vvysotskyi commented on code in PR #2655:
URL: https://github.com/apache/drill/pull/2655#discussion_r980305587
##
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java:
##
@@ -1094,6 +1095,35 @@ public static String bootDefaultFor(String name) {
new OptionDescription("Enables recursive files listing when querying the
`INFORMATION_SCHEMA.FILES` table or executing the SHOW FILES command. " +
"Default is false. (Drill 1.15+)"));
+ public static final String STORAGE_PLUGIN_ACCESS_ATTEMPTS =
"storage.plugin_access_attempts";
+ public static final PositiveLongValidator
STORAGE_PLUGIN_ACCESS_ATTEMPTS_VALIDATOR = new PositiveLongValidator(
+STORAGE_PLUGIN_ACCESS_ATTEMPTS,
+10,
+new OptionDescription(
+ "The maximum number of attempts that will be made to request metadata " +
+ "needed for query planning from a storage plugin."
+)
+ );
+ public static final String STORAGE_PLUGIN_ATTEMPT_DELAY =
"storage.plugin_access_attempt_delay";
+ public static final NonNegativeLongValidator
STORAGE_PLUGIN_ATTEMPT_DELAY_VALIDATOR = new NonNegativeLongValidator(
+STORAGE_PLUGIN_ATTEMPT_DELAY,
+5*60*1000,
Review Comment:
I meant to add missing spaces.
> Add support for automatically retrying and disabling broken storage plugins
> ---
>
> Key: DRILL-8314
> URL: https://issues.apache.org/jira/browse/DRILL-8314
> Project: Apache Drill
> Issue Type: Improvement
> Components: Storage - Other
>Affects Versions: 1.20.2
>Reporter: James Turton
>Assignee: James Turton
>Priority: Minor
> Fix For: 2.0.0
>
>
> Enabled storage plugins may malfunction for different reasons, e.g. they have
> been misconfigured or their remote data source has gone offline. Depending on
> the plugin's implementation, this could cause it to fail to return optimizer
> rules or register schemas. In some cases this can have a wider impact, e.g.
> unconditioned queries against the info schema will fail if a single plugin is
> failing in registerSchemas.
> Rather than us swallowing such errors and silently returning a subset of
> results, this Jira proposes new features that allow firstly for the retrying
> of attempts to get rules or schemas from a plugin and secondly for a plugin
> to be automatically disabled after the configured maximum number of attempts
> have failed. A broken plugin will still cause a query failure but the user
> will be informed that the plugin has been disabled as a result. Knowing what
> has just happened, the user can choose to reissue the query knowing that the
> broken plugin is now disabled or they might choose to investigate the problem
> affecting the broken plugin instead.
> These new features are optional and can be disabled using new SYSTEM options.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[
https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17609428#comment-17609428
]
ASF GitHub Bot commented on DRILL-8314:
---
jnturton commented on code in PR #2655:
URL: https://github.com/apache/drill/pull/2655#discussion_r979835243
##
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java:
##
@@ -1094,6 +1095,35 @@ public static String bootDefaultFor(String name) {
new OptionDescription("Enables recursive files listing when querying the
`INFORMATION_SCHEMA.FILES` table or executing the SHOW FILES command. " +
"Default is false. (Drill 1.15+)"));
+ public static final String STORAGE_PLUGIN_ACCESS_ATTEMPTS =
"storage.plugin_access_attempts";
+ public static final PositiveLongValidator
STORAGE_PLUGIN_ACCESS_ATTEMPTS_VALIDATOR = new PositiveLongValidator(
+STORAGE_PLUGIN_ACCESS_ATTEMPTS,
+10,
+new OptionDescription(
+ "The maximum number of attempts that will be made to request metadata " +
+ "needed for query planning from a storage plugin."
+)
+ );
+ public static final String STORAGE_PLUGIN_ATTEMPT_DELAY =
"storage.plugin_access_attempt_delay";
+ public static final NonNegativeLongValidator
STORAGE_PLUGIN_ATTEMPT_DELAY_VALIDATOR = new NonNegativeLongValidator(
+STORAGE_PLUGIN_ATTEMPT_DELAY,
+5*60*1000,
Review Comment:
Done. I asked IDEA to reformat the code and it indented the subsequent lines
in the string additions by one level. Let me know if that's what you meant, or
if you meant a bigger reformatting.
> Add support for automatically retrying and disabling broken storage plugins
> ---
>
> Key: DRILL-8314
> URL: https://issues.apache.org/jira/browse/DRILL-8314
> Project: Apache Drill
> Issue Type: Improvement
> Components: Storage - Other
>Affects Versions: 1.20.2
>Reporter: James Turton
>Assignee: James Turton
>Priority: Minor
> Fix For: 2.0.0
>
>
> Enabled storage plugins may malfunction for different reasons, e.g. they have
> been misconfigured or their remote data source has gone offline. Depending on
> the plugin's implementation, this could cause it to fail to return optimizer
> rules or register schemas. In some cases this can have a wider impact, e.g.
> unconditioned queries against the info schema will fail if a single plugin is
> failing in registerSchemas.
> Rather than us swallowing such errors and silently returning a subset of
> results, this Jira proposes new features that allow firstly for the retrying
> of attempts to get rules or schemas from a plugin and secondly for a plugin
> to be automatically disabled after the configured maximum number of attempts
> have failed. A broken plugin will still cause a query failure but the user
> will be informed that the plugin has been disabled as a result. Knowing what
> has just happened, the user can choose to reissue the query knowing that the
> broken plugin is now disabled or they might choose to investigate the problem
> affecting the broken plugin instead.
> These new features are optional and can be disabled using new SYSTEM options.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[
https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17609429#comment-17609429
]
ASF GitHub Bot commented on DRILL-8314:
---
jnturton commented on code in PR #2655:
URL: https://github.com/apache/drill/pull/2655#discussion_r979835243
##
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java:
##
@@ -1094,6 +1095,35 @@ public static String bootDefaultFor(String name) {
new OptionDescription("Enables recursive files listing when querying the
`INFORMATION_SCHEMA.FILES` table or executing the SHOW FILES command. " +
"Default is false. (Drill 1.15+)"));
+ public static final String STORAGE_PLUGIN_ACCESS_ATTEMPTS =
"storage.plugin_access_attempts";
+ public static final PositiveLongValidator
STORAGE_PLUGIN_ACCESS_ATTEMPTS_VALIDATOR = new PositiveLongValidator(
+STORAGE_PLUGIN_ACCESS_ATTEMPTS,
+10,
+new OptionDescription(
+ "The maximum number of attempts that will be made to request metadata " +
+ "needed for query planning from a storage plugin."
+)
+ );
+ public static final String STORAGE_PLUGIN_ATTEMPT_DELAY =
"storage.plugin_access_attempt_delay";
+ public static final NonNegativeLongValidator
STORAGE_PLUGIN_ATTEMPT_DELAY_VALIDATOR = new NonNegativeLongValidator(
+STORAGE_PLUGIN_ATTEMPT_DELAY,
+5*60*1000,
Review Comment:
Done. I asked IDEA to reformat the code and it indented the subsequent lines
in the string additions by one level. Let me know if you meant a bigger
reformatting.
> Add support for automatically retrying and disabling broken storage plugins
> ---
>
> Key: DRILL-8314
> URL: https://issues.apache.org/jira/browse/DRILL-8314
> Project: Apache Drill
> Issue Type: Improvement
> Components: Storage - Other
>Affects Versions: 1.20.2
>Reporter: James Turton
>Assignee: James Turton
>Priority: Minor
> Fix For: 2.0.0
>
>
> Enabled storage plugins may malfunction for different reasons, e.g. they have
> been misconfigured or their remote data source has gone offline. Depending on
> the plugin's implementation, this could cause it to fail to return optimizer
> rules or register schemas. In some cases this can have a wider impact, e.g.
> unconditioned queries against the info schema will fail if a single plugin is
> failing in registerSchemas.
> Rather than us swallowing such errors and silently returning a subset of
> results, this Jira proposes new features that allow firstly for the retrying
> of attempts to get rules or schemas from a plugin and secondly for a plugin
> to be automatically disabled after the configured maximum number of attempts
> have failed. A broken plugin will still cause a query failure but the user
> will be informed that the plugin has been disabled as a result. Knowing what
> has just happened, the user can choose to reissue the query knowing that the
> broken plugin is now disabled or they might choose to investigate the problem
> affecting the broken plugin instead.
> These new features are optional and can be disabled using new SYSTEM options.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[
https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17609425#comment-17609425
]
ASF GitHub Bot commented on DRILL-8314:
---
jnturton commented on code in PR #2655:
URL: https://github.com/apache/drill/pull/2655#discussion_r979833628
##
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java:
##
@@ -1094,6 +1095,35 @@ public static String bootDefaultFor(String name) {
new OptionDescription("Enables recursive files listing when querying the
`INFORMATION_SCHEMA.FILES` table or executing the SHOW FILES command. " +
"Default is false. (Drill 1.15+)"));
+ public static final String STORAGE_PLUGIN_ACCESS_ATTEMPTS =
"storage.plugin_access_attempts";
+ public static final PositiveLongValidator
STORAGE_PLUGIN_ACCESS_ATTEMPTS_VALIDATOR = new PositiveLongValidator(
+STORAGE_PLUGIN_ACCESS_ATTEMPTS,
+10,
+new OptionDescription(
+ "The maximum number of attempts that will be made to request metadata " +
+ "needed for query planning from a storage plugin."
+)
+ );
+ public static final String STORAGE_PLUGIN_ATTEMPT_DELAY =
"storage.plugin_access_attempt_delay";
+ public static final NonNegativeLongValidator
STORAGE_PLUGIN_ATTEMPT_DELAY_VALIDATOR = new NonNegativeLongValidator(
+STORAGE_PLUGIN_ATTEMPT_DELAY,
+5*60*1000,
+new OptionDescription(
+ "The delay in milliseconds between repeated attempts to request metadata
" +
Review Comment:
@vvysotskyi I did implement this that way initially but if you look I ended
up making the two features - storage plugin retry and storage plugin auto
disable - independent of each other here. Users could choose to have retry and
no auto disable, or to have auto disable and no retry the way it is currently.
> Add support for automatically retrying and disabling broken storage plugins
> ---
>
> Key: DRILL-8314
> URL: https://issues.apache.org/jira/browse/DRILL-8314
> Project: Apache Drill
> Issue Type: Improvement
> Components: Storage - Other
>Affects Versions: 1.20.2
>Reporter: James Turton
>Assignee: James Turton
>Priority: Minor
> Fix For: 2.0.0
>
>
> Enabled storage plugins may malfunction for different reasons, e.g. they have
> been misconfigured or their remote data source has gone offline. Depending on
> the plugin's implementation, this could cause it to fail to return optimizer
> rules or register schemas. In some cases this can have a wider impact, e.g.
> unconditioned queries against the info schema will fail if a single plugin is
> failing in registerSchemas.
> Rather than us swallowing such errors and silently returning a subset of
> results, this Jira proposes new features that allow firstly for the retrying
> of attempts to get rules or schemas from a plugin and secondly for a plugin
> to be automatically disabled after the configured maximum number of attempts
> have failed. A broken plugin will still cause a query failure but the user
> will be informed that the plugin has been disabled as a result. Knowing what
> has just happened, the user can choose to reissue the query knowing that the
> broken plugin is now disabled or they might choose to investigate the problem
> affecting the broken plugin instead.
> These new features are optional and can be disabled using new SYSTEM options.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[ https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17609423#comment-17609423 ] ASF GitHub Bot commented on DRILL-8314: --- jnturton commented on code in PR #2655: URL: https://github.com/apache/drill/pull/2655#discussion_r979823862 ## exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java: ## @@ -34,12 +34,22 @@ public interface StoragePluginRegistry extends Iterable Add support for automatically retrying and disabling broken storage plugins > --- > > Key: DRILL-8314 > URL: https://issues.apache.org/jira/browse/DRILL-8314 > Project: Apache Drill > Issue Type: Improvement > Components: Storage - Other >Affects Versions: 1.20.2 >Reporter: James Turton >Assignee: James Turton >Priority: Minor > Fix For: 2.0.0 > > > Enabled storage plugins may malfunction for different reasons, e.g. they have > been misconfigured or their remote data source has gone offline. Depending on > the plugin's implementation, this could cause it to fail to return optimizer > rules or register schemas. In some cases this can have a wider impact, e.g. > unconditioned queries against the info schema will fail if a single plugin is > failing in registerSchemas. > Rather than us swallowing such errors and silently returning a subset of > results, this Jira proposes new features that allow firstly for the retrying > of attempts to get rules or schemas from a plugin and secondly for a plugin > to be automatically disabled after the configured maximum number of attempts > have failed. A broken plugin will still cause a query failure but the user > will be informed that the plugin has been disabled as a result. Knowing what > has just happened, the user can choose to reissue the query knowing that the > broken plugin is now disabled or they might choose to investigate the problem > affecting the broken plugin instead. > These new features are optional and can be disabled using new SYSTEM options. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[
https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17609418#comment-17609418
]
ASF GitHub Bot commented on DRILL-8314:
---
jnturton commented on code in PR #2655:
URL: https://github.com/apache/drill/pull/2655#discussion_r979818742
##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SqlHandlerConfig.java:
##
@@ -52,12 +59,81 @@ public QueryContext getContext() {
return context;
}
+ private RuleSet attemptToGetRules(PlannerPhase phase,
Collection plugins) throws PluginException{
Review Comment:
@vvysotskyi so do we keep auto disabling but drop retry here? We've seen
JDBC plugins that point to an unresponsive remote DB fail with the first
exception coming from rule collection (because, unlike most plugins, JDBC
already tries to contact the remote DB for this operation). And of course
before DRILL-8234, JDBC plugins not even involved in a query could cause it to
fail during rule collection.
> Add support for automatically retrying and disabling broken storage plugins
> ---
>
> Key: DRILL-8314
> URL: https://issues.apache.org/jira/browse/DRILL-8314
> Project: Apache Drill
> Issue Type: Improvement
> Components: Storage - Other
>Affects Versions: 1.20.2
>Reporter: James Turton
>Assignee: James Turton
>Priority: Minor
> Fix For: 2.0.0
>
>
> Enabled storage plugins may malfunction for different reasons, e.g. they have
> been misconfigured or their remote data source has gone offline. Depending on
> the plugin's implementation, this could cause it to fail to return optimizer
> rules or register schemas. In some cases this can have a wider impact, e.g.
> unconditioned queries against the info schema will fail if a single plugin is
> failing in registerSchemas.
> Rather than us swallowing such errors and silently returning a subset of
> results, this Jira proposes new features that allow firstly for the retrying
> of attempts to get rules or schemas from a plugin and secondly for a plugin
> to be automatically disabled after the configured maximum number of attempts
> have failed. A broken plugin will still cause a query failure but the user
> will be informed that the plugin has been disabled as a result. Knowing what
> has just happened, the user can choose to reissue the query knowing that the
> broken plugin is now disabled or they might choose to investigate the problem
> affecting the broken plugin instead.
> These new features are optional and can be disabled using new SYSTEM options.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[
https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17609415#comment-17609415
]
ASF GitHub Bot commented on DRILL-8314:
---
jnturton commented on code in PR #2655:
URL: https://github.com/apache/drill/pull/2655#discussion_r979810768
##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java:
##
@@ -130,11 +132,18 @@ private static PhysicalPlan convertPlan(QueryContext
context, String sql, Pointe
logger.trace("There was an error during conversion into physical plan. "
+
"Will sync remote and local function registries if needed and retry
" +
"in case if issue was due to missing function implementation.", e);
- // it is prohibited to retry query planning for ANALYZE statement since
it changes
- // query-level option values and will fail when rerunning with updated
values
- if (context.getFunctionRegistry().syncWithRemoteRegistry(
- context.getDrillOperatorTable().getFunctionRegistryVersion())
-&& context.getSQLStatementType() != SqlStatementType.ANALYZE) {
+
+ int funcRegVer =
context.getDrillOperatorTable().getFunctionRegistryVersion();
+ // We do not retry conversion if the error is a UserException of type
RESOURCE
+ boolean isResourceErr = e instanceof UserException && ((UserException)
e).getErrorType() == RESOURCE;
Review Comment:
@vvysotskyi when plugin auto disabling is switched on then this second
attempt to convert the query involving a broken plugin fails with an error of
"schema not found" because the plugin has been disabled. This error isn't as
informative to the user as the original error raised when attempting to access
the plugin so I didn't want this to be the error that they get back...
> Add support for automatically retrying and disabling broken storage plugins
> ---
>
> Key: DRILL-8314
> URL: https://issues.apache.org/jira/browse/DRILL-8314
> Project: Apache Drill
> Issue Type: Improvement
> Components: Storage - Other
>Affects Versions: 1.20.2
>Reporter: James Turton
>Assignee: James Turton
>Priority: Minor
> Fix For: 2.0.0
>
>
> Enabled storage plugins may malfunction for different reasons, e.g. they have
> been misconfigured or their remote data source has gone offline. Depending on
> the plugin's implementation, this could cause it to fail to return optimizer
> rules or register schemas. In some cases this can have a wider impact, e.g.
> unconditioned queries against the info schema will fail if a single plugin is
> failing in registerSchemas.
> Rather than us swallowing such errors and silently returning a subset of
> results, this Jira proposes new features that allow firstly for the retrying
> of attempts to get rules or schemas from a plugin and secondly for a plugin
> to be automatically disabled after the configured maximum number of attempts
> have failed. A broken plugin will still cause a query failure but the user
> will be informed that the plugin has been disabled as a result. Knowing what
> has just happened, the user can choose to reissue the query knowing that the
> broken plugin is now disabled or they might choose to investigate the problem
> affecting the broken plugin instead.
> These new features are optional and can be disabled using new SYSTEM options.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[
https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17609414#comment-17609414
]
ASF GitHub Bot commented on DRILL-8314:
---
jnturton commented on code in PR #2655:
URL: https://github.com/apache/drill/pull/2655#discussion_r979810768
##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java:
##
@@ -130,11 +132,18 @@ private static PhysicalPlan convertPlan(QueryContext
context, String sql, Pointe
logger.trace("There was an error during conversion into physical plan. "
+
"Will sync remote and local function registries if needed and retry
" +
"in case if issue was due to missing function implementation.", e);
- // it is prohibited to retry query planning for ANALYZE statement since
it changes
- // query-level option values and will fail when rerunning with updated
values
- if (context.getFunctionRegistry().syncWithRemoteRegistry(
- context.getDrillOperatorTable().getFunctionRegistryVersion())
-&& context.getSQLStatementType() != SqlStatementType.ANALYZE) {
+
+ int funcRegVer =
context.getDrillOperatorTable().getFunctionRegistryVersion();
+ // We do not retry conversion if the error is a UserException of type
RESOURCE
+ boolean isResourceErr = e instanceof UserException && ((UserException)
e).getErrorType() == RESOURCE;
Review Comment:
@vvysotskyi when plugin auto disabling is switched on then this second
attempt to convert the query fails with an error of "schema not found" because
the plugin has been disabled. This error isn't as informative to the user as
the original error raised when attempting to access the plugin so I didn't want
this to be the error that they get back...
> Add support for automatically retrying and disabling broken storage plugins
> ---
>
> Key: DRILL-8314
> URL: https://issues.apache.org/jira/browse/DRILL-8314
> Project: Apache Drill
> Issue Type: Improvement
> Components: Storage - Other
>Affects Versions: 1.20.2
>Reporter: James Turton
>Assignee: James Turton
>Priority: Minor
> Fix For: 2.0.0
>
>
> Enabled storage plugins may malfunction for different reasons, e.g. they have
> been misconfigured or their remote data source has gone offline. Depending on
> the plugin's implementation, this could cause it to fail to return optimizer
> rules or register schemas. In some cases this can have a wider impact, e.g.
> unconditioned queries against the info schema will fail if a single plugin is
> failing in registerSchemas.
> Rather than us swallowing such errors and silently returning a subset of
> results, this Jira proposes new features that allow firstly for the retrying
> of attempts to get rules or schemas from a plugin and secondly for a plugin
> to be automatically disabled after the configured maximum number of attempts
> have failed. A broken plugin will still cause a query failure but the user
> will be informed that the plugin has been disabled as a result. Knowing what
> has just happened, the user can choose to reissue the query knowing that the
> broken plugin is now disabled or they might choose to investigate the problem
> affecting the broken plugin instead.
> These new features are optional and can be disabled using new SYSTEM options.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[
https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17609305#comment-17609305
]
ASF GitHub Bot commented on DRILL-8314:
---
vvysotskyi commented on code in PR #2655:
URL: https://github.com/apache/drill/pull/2655#discussion_r979578252
##
exec/java-exec/src/main/java/org/apache/calcite/jdbc/DynamicRootSchema.java:
##
@@ -91,22 +91,69 @@ private CalciteSchema getSchema(String schemaName, boolean
caseSensitive) {
public SchemaPath resolveTableAlias(String alias) {
return Optional.ofNullable(aliasRegistryProvider.getTableAliasesRegistry()
-.getUserAliases(schemaConfig.getUserName()).get(alias))
+ .getUserAliases(schemaConfig.getUserName()).get(alias))
.map(SchemaPath::parseFromString)
.orElse(null);
}
+ private void attemptToRegisterSchemas(StoragePlugin plugin) throws Exception
{
Review Comment:
Please rename the method to something like register with retry.
##
exec/java-exec/src/main/java/org/apache/calcite/jdbc/DynamicRootSchema.java:
##
@@ -91,22 +91,69 @@ private CalciteSchema getSchema(String schemaName, boolean
caseSensitive) {
public SchemaPath resolveTableAlias(String alias) {
return Optional.ofNullable(aliasRegistryProvider.getTableAliasesRegistry()
-.getUserAliases(schemaConfig.getUserName()).get(alias))
+ .getUserAliases(schemaConfig.getUserName()).get(alias))
.map(SchemaPath::parseFromString)
.orElse(null);
}
+ private void attemptToRegisterSchemas(StoragePlugin plugin) throws Exception
{
+long maxAttempts = schemaConfig
+ .getOption(ExecConstants.STORAGE_PLUGIN_ACCESS_ATTEMPTS)
+ .num_val;
+long attemptDelayMs = schemaConfig
+ .getOption(ExecConstants.STORAGE_PLUGIN_ATTEMPT_DELAY)
+ .num_val;
+int attempt=0;
+Exception lastAttemptEx = null;
+
+while (attempt++ < maxAttempts) {
Review Comment:
Could we either move retry-related logic to a utility method or use some
library that does that, for example, https://github.com/failsafe-lib/failsafe?
##
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java:
##
@@ -1094,6 +1095,35 @@ public static String bootDefaultFor(String name) {
new OptionDescription("Enables recursive files listing when querying the
`INFORMATION_SCHEMA.FILES` table or executing the SHOW FILES command. " +
"Default is false. (Drill 1.15+)"));
+ public static final String STORAGE_PLUGIN_ACCESS_ATTEMPTS =
"storage.plugin_access_attempts";
+ public static final PositiveLongValidator
STORAGE_PLUGIN_ACCESS_ATTEMPTS_VALIDATOR = new PositiveLongValidator(
+STORAGE_PLUGIN_ACCESS_ATTEMPTS,
+10,
+new OptionDescription(
+ "The maximum number of attempts that will be made to request metadata " +
+ "needed for query planning from a storage plugin."
+)
+ );
+ public static final String STORAGE_PLUGIN_ATTEMPT_DELAY =
"storage.plugin_access_attempt_delay";
+ public static final NonNegativeLongValidator
STORAGE_PLUGIN_ATTEMPT_DELAY_VALIDATOR = new NonNegativeLongValidator(
+STORAGE_PLUGIN_ATTEMPT_DELAY,
+5*60*1000,
+new OptionDescription(
+ "The delay in milliseconds between repeated attempts to request metadata
" +
Review Comment:
Please mention here and in the option above that they will be used only when
`storage.plugin_auto_disable` is set to true.
##
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java:
##
@@ -130,11 +132,18 @@ private static PhysicalPlan convertPlan(QueryContext
context, String sql, Pointe
logger.trace("There was an error during conversion into physical plan. "
+
"Will sync remote and local function registries if needed and retry
" +
"in case if issue was due to missing function implementation.", e);
- // it is prohibited to retry query planning for ANALYZE statement since
it changes
- // query-level option values and will fail when rerunning with updated
values
- if (context.getFunctionRegistry().syncWithRemoteRegistry(
- context.getDrillOperatorTable().getFunctionRegistryVersion())
-&& context.getSQLStatementType() != SqlStatementType.ANALYZE) {
+
+ int funcRegVer =
context.getDrillOperatorTable().getFunctionRegistryVersion();
+ // We do not retry conversion if the error is a UserException of type
RESOURCE
+ boolean isResourceErr = e instanceof UserException && ((UserException)
e).getErrorType() == RESOURCE;
Review Comment:
I don't think that it wouldn't break the initial intention for retrying the
query after updating the function registry. It is possible that some part of
the code wraps exceptions caused by absent functions as `UserException` with
`RESOURCE` type.
##
exec/java-exec/src/main/java/org/apache/cal
[jira] [Commented] (DRILL-8314) Add support for automatically retrying and disabling broken storage plugins
[ https://issues.apache.org/jira/browse/DRILL-8314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17608592#comment-17608592 ] ASF GitHub Bot commented on DRILL-8314: --- jnturton commented on PR #2655: URL: https://github.com/apache/drill/pull/2655#issuecomment-1255872622 Switched to Draft to prevent merging because the second reviewer has requested an opportunity to review (likely on Monday). > Add support for automatically retrying and disabling broken storage plugins > --- > > Key: DRILL-8314 > URL: https://issues.apache.org/jira/browse/DRILL-8314 > Project: Apache Drill > Issue Type: Improvement > Components: Storage - Other >Affects Versions: 1.20.2 >Reporter: James Turton >Assignee: James Turton >Priority: Minor > Fix For: 2.0.0 > > > Enabled storage plugins may malfunction for different reasons, e.g. they have > been misconfigured or their remote data source has gone offline. Depending on > the plugin's implementation, this could cause it to fail to return optimizer > rules or register schemas. In some cases this can have a wider impact, e.g. > unconditioned queries against the info schema will fail if a single plugin is > failing in registerSchemas. > Rather than us swallowing such errors and silently returning a subset of > results, this Jira proposes new features that allow firstly for the retrying > of attempts to get rules or schemas from a plugin and secondly for a plugin > to be automatically disabled after the configured maximum number of attempts > have failed. A broken plugin will still cause a query failure but the user > will be informed that the plugin has been disabled as a result. Knowing what > has just happened, the user can choose to reissue the query knowing that the > broken plugin is now disabled or they might choose to investigate the problem > affecting the broken plugin instead. > These new features are optional and can be disabled using new SYSTEM options. -- This message was sent by Atlassian Jira (v8.20.10#820010)
