[GitHub] drill pull request #1013: DRILL-5910: Add factory name to ClassNotFoundExcep...
Github user vladimirtkach commented on a diff in the pull request:
https://github.com/apache/drill/pull/1013#discussion_r147655905
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/ClientAuthenticatorProvider.java
---
@@ -57,17 +57,17 @@ private ClientAuthenticatorProvider() {
// then, custom factories
if (customFactories != null) {
- try {
-final String[] factories = customFactories.split(",");
-for (final String factory : factories) {
+ final String[] factories = customFactories.split(",");
+ for (final String factory : factories) {
+try {
final Class clazz = Class.forName(factory);
if (AuthenticatorFactory.class.isAssignableFrom(clazz)) {
final AuthenticatorFactory instance = (AuthenticatorFactory)
clazz.newInstance();
authFactories.put(instance.getSimpleName(), instance);
}
+} catch (final ClassNotFoundException | IllegalAccessException |
InstantiationException e) {
--- End diff --
a moot point, I think that three specific exceptions is better the one
general.
---
[GitHub] drill pull request #1013: DRILL-5910: Add factory name to ClassNotFoundExcep...
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/1013#discussion_r147541442
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/ClientAuthenticatorProvider.java
---
@@ -57,17 +57,17 @@ private ClientAuthenticatorProvider() {
// then, custom factories
if (customFactories != null) {
- try {
-final String[] factories = customFactories.split(",");
-for (final String factory : factories) {
+ final String[] factories = customFactories.split(",");
+ for (final String factory : factories) {
+try {
final Class clazz = Class.forName(factory);
if (AuthenticatorFactory.class.isAssignableFrom(clazz)) {
final AuthenticatorFactory instance = (AuthenticatorFactory)
clazz.newInstance();
authFactories.put(instance.getSimpleName(), instance);
}
+} catch (final ClassNotFoundException | IllegalAccessException |
InstantiationException e) {
+ throw new DrillRuntimeException(String.format("Failed to create
auth factory '%s'", factory), e);
--- End diff --
@vrozov +1 - It makes sense to log the error and continue rather than
failing because a custom factory cannot be instantiated. This is because client
environment might be set to configure these custom factories but then for
authentication it might be using other mechanism like Plain/Kerberos in which
case connection should succeed.
---
[GitHub] drill pull request #1013: DRILL-5910: Add factory name to ClassNotFoundExcep...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1013#discussion_r147466798
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/ClientAuthenticatorProvider.java
---
@@ -57,17 +57,17 @@ private ClientAuthenticatorProvider() {
// then, custom factories
if (customFactories != null) {
- try {
-final String[] factories = customFactories.split(",");
-for (final String factory : factories) {
+ final String[] factories = customFactories.split(",");
+ for (final String factory : factories) {
+try {
final Class clazz = Class.forName(factory);
if (AuthenticatorFactory.class.isAssignableFrom(clazz)) {
final AuthenticatorFactory instance = (AuthenticatorFactory)
clazz.newInstance();
authFactories.put(instance.getSimpleName(), instance);
}
+} catch (final ClassNotFoundException | IllegalAccessException |
InstantiationException e) {
+ throw new DrillRuntimeException(String.format("Failed to create
auth factory '%s'", factory), e);
--- End diff --
It will be good to explain why. In other systems (for example unix pam)
usually, there is a best effort attempt to authenticate, meaning that if a
provider can't be initialized or instantiated it is skipped, but the system is
operational and users that authenticate against other providers can still use
the system.
---
[GitHub] drill pull request #1013: DRILL-5910: Add factory name to ClassNotFoundExcep...
Github user prasadns14 commented on a diff in the pull request:
https://github.com/apache/drill/pull/1013#discussion_r147463501
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/ClientAuthenticatorProvider.java
---
@@ -57,17 +57,17 @@ private ClientAuthenticatorProvider() {
// then, custom factories
if (customFactories != null) {
- try {
-final String[] factories = customFactories.split(",");
-for (final String factory : factories) {
+ final String[] factories = customFactories.split(",");
+ for (final String factory : factories) {
+try {
final Class clazz = Class.forName(factory);
if (AuthenticatorFactory.class.isAssignableFrom(clazz)) {
final AuthenticatorFactory instance = (AuthenticatorFactory)
clazz.newInstance();
authFactories.put(instance.getSimpleName(), instance);
}
+} catch (final ClassNotFoundException | IllegalAccessException |
InstantiationException e) {
+ throw new DrillRuntimeException(String.format("Failed to create
auth factory '%s'", factory), e);
--- End diff --
I don't think we should continue. When a custom factory is requested, in
this case a custom authenticator factory it doesn't make sense to continue if
it can't be instantiated.
---
[GitHub] drill pull request #1013: DRILL-5910: Add factory name to ClassNotFoundExcep...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1013#discussion_r147443404
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/ClientAuthenticatorProvider.java
---
@@ -57,17 +57,17 @@ private ClientAuthenticatorProvider() {
// then, custom factories
if (customFactories != null) {
- try {
-final String[] factories = customFactories.split(",");
-for (final String factory : factories) {
+ final String[] factories = customFactories.split(",");
+ for (final String factory : factories) {
+try {
final Class clazz = Class.forName(factory);
if (AuthenticatorFactory.class.isAssignableFrom(clazz)) {
final AuthenticatorFactory instance = (AuthenticatorFactory)
clazz.newInstance();
authFactories.put(instance.getSimpleName(), instance);
}
+} catch (final ClassNotFoundException | IllegalAccessException |
InstantiationException e) {
--- End diff --
catch ReflectiveOperationException.
---
[GitHub] drill pull request #1013: DRILL-5910: Add factory name to ClassNotFoundExcep...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1013#discussion_r147445561
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/ClientAuthenticatorProvider.java
---
@@ -57,17 +57,17 @@ private ClientAuthenticatorProvider() {
// then, custom factories
if (customFactories != null) {
- try {
-final String[] factories = customFactories.split(",");
-for (final String factory : factories) {
+ final String[] factories = customFactories.split(",");
+ for (final String factory : factories) {
+try {
final Class clazz = Class.forName(factory);
if (AuthenticatorFactory.class.isAssignableFrom(clazz)) {
final AuthenticatorFactory instance = (AuthenticatorFactory)
clazz.newInstance();
authFactories.put(instance.getSimpleName(), instance);
}
+} catch (final ClassNotFoundException | IllegalAccessException |
InstantiationException e) {
+ throw new DrillRuntimeException(String.format("Failed to create
auth factory '%s'", factory), e);
--- End diff --
Not directly related to this PR, but should it continue with the default
set of factories if one of custom factories can't be instantiated?
---
