You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by arina-ielchiieva <gi...@git.apache.org> on 2016/11/28 11:19:27 UTC
[GitHub] drill pull request #669: DRILL-5044: After the dynamic registration of multi...
GitHub user arina-ielchiieva opened a pull request:
https://github.com/apache/drill/pull/669
DRILL-5044: After the dynamic registration of multiple jars simultane\u2026
\u2026ously not all UDFs were registered
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/arina-ielchiieva/drill DRILL-5044
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/669.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #669
----
commit a5ee5549e834bfbf9142bfbf51ff37c421cc23db
Author: Arina Ielchiieva <ar...@gmail.com>
Date: 2016-11-25T16:44:08Z
DRILL-5044: After the dynamic registration of multiple jars simultaneously not all UDFs were registered
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #669: DRILL-5044: After the dynamic registration of multi...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/669#discussion_r89993367
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateFunctionHandler.java ---
@@ -175,22 +175,20 @@ private void initRemoteRegistration(List<String> functions,
List<Jar> remoteJars = remoteRegistry.getRegistry(version).getJarList();
validateAgainstRemoteRegistry(remoteJars, jarManager.getBinaryName(), functions);
jarManager.copyToRegistryArea();
- boolean cleanUp = true;
List<Jar> jars = Lists.newArrayList(remoteJars);
jars.add(Jar.newBuilder().setName(jarManager.getBinaryName()).addAllFunctionSignature(functions).build());
Registry updatedRegistry = Registry.newBuilder().addAllJar(jars).build();
try {
remoteRegistry.updateRegistry(updatedRegistry, version);
- cleanUp = false;
} catch (VersionMismatchException ex) {
+ jarManager.deleteQuietlyFromRegistryArea();
if (retryAttempts-- == 0) {
throw new DrillRuntimeException("Failed to update remote function registry. Exceeded retry attempts limit.");
}
initRemoteRegistration(functions, jarManager, remoteRegistry, retryAttempts);
- } finally {
- if (cleanUp) {
- jarManager.deleteQuietlyFromRegistryArea();
- }
+ } catch (Exception e) {
--- End diff --
You are right. Updated the code, so now we delete jars only once on error.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #669: DRILL-5044: After the dynamic registration of multi...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/669#discussion_r89848425
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DropFunctionHandler.java ---
@@ -143,7 +143,7 @@ private Jar unregister(String jarName, RemoteFunctionRegistry remoteFunctionRegi
if (retryAttempts-- == 0) {
throw new DrillRuntimeException("Failed to update remote function registry. Exceeded retry attempts limit.");
}
- unregister(jarName, remoteFunctionRegistry, retryAttempts);
+ return unregister(jarName, remoteFunctionRegistry, retryAttempts);
--- End diff --
Comments from registration process apply here as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #669: DRILL-5044: After the dynamic registration of multi...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/669#discussion_r89849203
--- Diff: exec/java-exec/src/test/java/org/apache/drill/TestDynamicUDFSupport.java ---
@@ -271,6 +271,75 @@ public void testDuplicatedFunctionsInLocalRegistry() throws Exception {
}
@Test
+ public void testSuccessfulRegistrationAfterSeveralRetryAttempts() throws Exception {
--- End diff --
The case in question would seem to occur when multiple registrations happen concurrently. Do we have a test for that case? That is, do we try to run two thread (two Drillbits?) and have them each attempt to register jars?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #669: DRILL-5044: After the dynamic registration of multi...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/669#discussion_r89848150
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateFunctionHandler.java ---
@@ -175,22 +175,20 @@ private void initRemoteRegistration(List<String> functions,
List<Jar> remoteJars = remoteRegistry.getRegistry(version).getJarList();
validateAgainstRemoteRegistry(remoteJars, jarManager.getBinaryName(), functions);
jarManager.copyToRegistryArea();
- boolean cleanUp = true;
List<Jar> jars = Lists.newArrayList(remoteJars);
jars.add(Jar.newBuilder().setName(jarManager.getBinaryName()).addAllFunctionSignature(functions).build());
Registry updatedRegistry = Registry.newBuilder().addAllJar(jars).build();
try {
remoteRegistry.updateRegistry(updatedRegistry, version);
- cleanUp = false;
} catch (VersionMismatchException ex) {
+ jarManager.deleteQuietlyFromRegistryArea();
if (retryAttempts-- == 0) {
throw new DrillRuntimeException("Failed to update remote function registry. Exceeded retry attempts limit.");
}
initRemoteRegistration(functions, jarManager, remoteRegistry, retryAttempts);
- } finally {
- if (cleanUp) {
- jarManager.deleteQuietlyFromRegistryArea();
- }
+ } catch (Exception e) {
--- End diff --
Since recursion is used on retry, won't we end up deleting the jar multiple times? No harm, but seems odd...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #669: DRILL-5044: After the dynamic registration of multi...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/669#discussion_r89993422
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DropFunctionHandler.java ---
@@ -143,7 +143,7 @@ private Jar unregister(String jarName, RemoteFunctionRegistry remoteFunctionRegi
if (retryAttempts-- == 0) {
throw new DrillRuntimeException("Failed to update remote function registry. Exceeded retry attempts limit.");
}
- unregister(jarName, remoteFunctionRegistry, retryAttempts);
+ return unregister(jarName, remoteFunctionRegistry, retryAttempts);
--- End diff --
Agree. Done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #669: DRILL-5044: After the dynamic registration of multi...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/669#discussion_r89848865
--- Diff: exec/java-exec/src/test/java/org/apache/drill/TestDynamicUDFSupport.java ---
@@ -271,6 +271,75 @@ public void testDuplicatedFunctionsInLocalRegistry() throws Exception {
}
@Test
+ public void testSuccessfulRegistrationAfterSeveralRetryAttempts() throws Exception {
+ RemoteFunctionRegistry remoteFunctionRegistry = spyRemoteFunctionRegistry();
+ copyDefaultJarsToStagingArea();
+
+ doThrow(new VersionMismatchException("Version mismatch detected", 1))
+ .doThrow(new VersionMismatchException("Version mismatch detected", 1))
+ .doCallRealMethod()
+ .when(remoteFunctionRegistry).updateRegistry(any(Registry.class), any(DataChangeVersion.class));
+
+ String summary = "The following UDFs in jar %s have been registered:\n" +
+ "[custom_lower(VARCHAR-REQUIRED)]";
+
+ testBuilder()
+ .sqlQuery("create function using jar '%s'", default_binary_name)
+ .unOrdered()
+ .baselineColumns("ok", "summary")
+ .baselineValues(true, String.format(summary, default_binary_name))
+ .go();
+
+ verify(remoteFunctionRegistry, times(3))
+ .updateRegistry(any(Registry.class), any(DataChangeVersion.class));
+
+ FileSystem fs = remoteFunctionRegistry.getFs();
+
+ assertFalse("Staging area should be empty", fs.listFiles(remoteFunctionRegistry.getStagingArea(), false).hasNext());
+ assertFalse("Temporary area should be empty", fs.listFiles(remoteFunctionRegistry.getTmpArea(), false).hasNext());
+
+ assertTrue("Binary should be present in registry area",
+ fs.exists(new Path(remoteFunctionRegistry.getRegistryArea(), default_binary_name)));
+ assertTrue("Source should be present in registry area",
+ fs.exists(new Path(remoteFunctionRegistry.getRegistryArea(), default_source_name)));
+
+ Registry registry = remoteFunctionRegistry.getRegistry();
+ assertEquals("Registry should contain one jar", registry.getJarList().size(), 1);
+ assertEquals(registry.getJar(0).getName(), default_binary_name);
+ }
+
+ @Test
+ public void testSuccessfulUnregistrationAfterSeveralRetryAttempts() throws Exception {
+ RemoteFunctionRegistry remoteFunctionRegistry = spyRemoteFunctionRegistry();
+ copyDefaultJarsToStagingArea();
+ test("create function using jar '%s'", default_binary_name);
+
+ reset(remoteFunctionRegistry);
+ doThrow(new VersionMismatchException("Version mismatch detected", 1))
--- End diff --
Perhaps a few words of explanation about what doThrow does?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #669: DRILL-5044: After the dynamic registration of multiple jar...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/669
@paul-rogers, I have made changes after CR, please review when possible.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #669: DRILL-5044: After the dynamic registration of multi...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/669#discussion_r89994171
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateFunctionHandler.java ---
@@ -175,22 +175,20 @@ private void initRemoteRegistration(List<String> functions,
List<Jar> remoteJars = remoteRegistry.getRegistry(version).getJarList();
validateAgainstRemoteRegistry(remoteJars, jarManager.getBinaryName(), functions);
jarManager.copyToRegistryArea();
- boolean cleanUp = true;
List<Jar> jars = Lists.newArrayList(remoteJars);
jars.add(Jar.newBuilder().setName(jarManager.getBinaryName()).addAllFunctionSignature(functions).build());
Registry updatedRegistry = Registry.newBuilder().addAllJar(jars).build();
try {
remoteRegistry.updateRegistry(updatedRegistry, version);
- cleanUp = false;
} catch (VersionMismatchException ex) {
+ jarManager.deleteQuietlyFromRegistryArea();
--- End diff --
1. I guess having fixed number of retries is enough. Having retry and wait logic, may lead us to the point where user will have to wait for a long time till registration completes in case of busy system. With only retry logic we notify user pretty quickly that the system is busy and it's up to the user to decide when to try to register the function again,
2. Totally agree about recursion, since user may modify number of retry attempts, it's much better to have while loop to avoid stack overflow.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #669: DRILL-5044: After the dynamic registration of multi...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/669#discussion_r89847859
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateFunctionHandler.java ---
@@ -175,22 +175,20 @@ private void initRemoteRegistration(List<String> functions,
List<Jar> remoteJars = remoteRegistry.getRegistry(version).getJarList();
validateAgainstRemoteRegistry(remoteJars, jarManager.getBinaryName(), functions);
jarManager.copyToRegistryArea();
- boolean cleanUp = true;
List<Jar> jars = Lists.newArrayList(remoteJars);
jars.add(Jar.newBuilder().setName(jarManager.getBinaryName()).addAllFunctionSignature(functions).build());
Registry updatedRegistry = Registry.newBuilder().addAllJar(jars).build();
try {
remoteRegistry.updateRegistry(updatedRegistry, version);
- cleanUp = false;
} catch (VersionMismatchException ex) {
+ jarManager.deleteQuietlyFromRegistryArea();
--- End diff --
I wonder about the retry approach. If we get a version mis-match, it means that something has changed: some other agent is modifying the registry at the same time. How should this be handed? Fixed number of retry? Wait and retry? Presumably, after some time, the system will get less busy, or we'll finally get our crack at the registry.
Also, is it wise to use recursion to retry? Doing so causes a stack depth proportional to the retry count, which may not be ideal. Loop instead?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #669: DRILL-5044: After the dynamic registration of multi...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/669
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #669: DRILL-5044: After the dynamic registration of multiple jar...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the issue:
https://github.com/apache/drill/pull/669
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #669: DRILL-5044: After the dynamic registration of multi...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/669#discussion_r89995323
--- Diff: exec/java-exec/src/test/java/org/apache/drill/TestDynamicUDFSupport.java ---
@@ -271,6 +271,75 @@ public void testDuplicatedFunctionsInLocalRegistry() throws Exception {
}
@Test
+ public void testSuccessfulRegistrationAfterSeveralRetryAttempts() throws Exception {
+ RemoteFunctionRegistry remoteFunctionRegistry = spyRemoteFunctionRegistry();
+ copyDefaultJarsToStagingArea();
+
+ doThrow(new VersionMismatchException("Version mismatch detected", 1))
+ .doThrow(new VersionMismatchException("Version mismatch detected", 1))
+ .doCallRealMethod()
+ .when(remoteFunctionRegistry).updateRegistry(any(Registry.class), any(DataChangeVersion.class));
+
+ String summary = "The following UDFs in jar %s have been registered:\n" +
+ "[custom_lower(VARCHAR-REQUIRED)]";
+
+ testBuilder()
+ .sqlQuery("create function using jar '%s'", default_binary_name)
+ .unOrdered()
+ .baselineColumns("ok", "summary")
+ .baselineValues(true, String.format(summary, default_binary_name))
+ .go();
+
+ verify(remoteFunctionRegistry, times(3))
+ .updateRegistry(any(Registry.class), any(DataChangeVersion.class));
+
+ FileSystem fs = remoteFunctionRegistry.getFs();
+
+ assertFalse("Staging area should be empty", fs.listFiles(remoteFunctionRegistry.getStagingArea(), false).hasNext());
+ assertFalse("Temporary area should be empty", fs.listFiles(remoteFunctionRegistry.getTmpArea(), false).hasNext());
+
+ assertTrue("Binary should be present in registry area",
+ fs.exists(new Path(remoteFunctionRegistry.getRegistryArea(), default_binary_name)));
+ assertTrue("Source should be present in registry area",
+ fs.exists(new Path(remoteFunctionRegistry.getRegistryArea(), default_source_name)));
+
+ Registry registry = remoteFunctionRegistry.getRegistry();
+ assertEquals("Registry should contain one jar", registry.getJarList().size(), 1);
+ assertEquals(registry.getJar(0).getName(), default_binary_name);
+ }
+
+ @Test
+ public void testSuccessfulUnregistrationAfterSeveralRetryAttempts() throws Exception {
+ RemoteFunctionRegistry remoteFunctionRegistry = spyRemoteFunctionRegistry();
+ copyDefaultJarsToStagingArea();
+ test("create function using jar '%s'", default_binary_name);
+
+ reset(remoteFunctionRegistry);
+ doThrow(new VersionMismatchException("Version mismatch detected", 1))
--- End diff --
It's Mockito functionality. You can mock the method to return failure or any result when it is being called.
In this case we mock `updateRegistry()` method to return VersionMismatchException first two times. This way we simulate the situation that someone has updated remote function registry before us.
After that we instruct to call real method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #669: DRILL-5044: After the dynamic registration of multi...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/669#discussion_r89994657
--- Diff: exec/java-exec/src/test/java/org/apache/drill/TestDynamicUDFSupport.java ---
@@ -271,6 +271,75 @@ public void testDuplicatedFunctionsInLocalRegistry() throws Exception {
}
@Test
+ public void testSuccessfulRegistrationAfterSeveralRetryAttempts() throws Exception {
--- End diff --
Actually test `testSuccessfulRegistrationAfterSeveralRetryAttempts()` covers this case. To produce this issue we don't need several drillbits, it's just enough to issue several concurrent create function commands. As for this test case, I just mock method that updates remote function registry to throw VersionMismatchException two times and then call this method for real.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---