You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2022/09/09 11:41:46 UTC
[GitHub] [zeppelin] huage1994 opened a new pull request, #4463: Remove prefix in jdbc interpreter
huage1994 opened a new pull request, #4463:
URL: https://github.com/apache/zeppelin/pull/4463
### What is this PR for?
Currently, zeppelin allow user to run multiple kinds of sql in one interpreter, e.g.
```
%jdbc(db=mysql)
%jdbc(db=hive){code}
```
But this would make jdbc interpreter very complicated, and hard to maintain.
This PR is to proposal to remove this feature, so that user need to create separated interpreter for each database.
```
%mysql
%hive
```
### What type of PR is it?
Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
* Open an issue on Jira [ZEPPELIN-5493](https://issues.apache.org/jira/browse/ZEPPELIN-5493)
### How should this be tested?
CI passed
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4463: [ZEPPELIN-5493] Remove prefix in jdbc interpreter
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4463:
URL: https://github.com/apache/zeppelin/pull/4463#discussion_r983477191
##########
jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java:
##########
@@ -550,9 +549,9 @@ public Connection getConnection(String dbPrefix, InterpreterContext context)
LOGGER.debug("createSecureConfiguration() returned");
boolean isProxyEnabled = Boolean.parseBoolean(
getProperty("zeppelin.jdbc.auth.kerberos.proxy.enable", "true"));
- if (basePropertiesMap.get(dbPrefix).containsKey("proxy.user.property")
+ if (basePropertiesMap.get(DEFAULT_KEY).containsKey("proxy.user.property")
Review Comment:
BTW, can we change `basePropertiesMap` not to use DEFAULT_KEY as a key?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on a diff in pull request #4463: [ZEPPELIN-5493] Remove prefix in jdbc interpreter
Posted by GitBox <gi...@apache.org>.
huage1994 commented on code in PR #4463:
URL: https://github.com/apache/zeppelin/pull/4463#discussion_r985214869
##########
jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java:
##########
@@ -550,9 +549,9 @@ public Connection getConnection(String dbPrefix, InterpreterContext context)
LOGGER.debug("createSecureConfiguration() returned");
boolean isProxyEnabled = Boolean.parseBoolean(
getProperty("zeppelin.jdbc.auth.kerberos.proxy.enable", "true"));
- if (basePropertiesMap.get(dbPrefix).containsKey("proxy.user.property")
+ if (basePropertiesMap.get(DEFAULT_KEY).containsKey("proxy.user.property")
Review Comment:
> BTW, can we change `basePropertiesMap` not to use DEFAULT_KEY as a key?
Hi @jongyoul ,I totally agree with you.
I plan to refactor `JDBC` module in the very soon future.
As I was developing, I realized that removing `basePropertiesMap` would require a significant code change. I don't want this PR to get too big , so I plan to refactor `JDBC` module in another PR and do more verification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4463: [ZEPPELIN-5493] Remove prefix in jdbc interpreter
Posted by GitBox <gi...@apache.org>.
huage1994 commented on PR #4463:
URL: https://github.com/apache/zeppelin/pull/4463#issuecomment-1291490814
Hi @jongyoul , thanks a lot for your review! Is there any code I need to change?
`Merging can be performed automatically once the requested changes are addressed.`
It seems that `change requested` state can only be addressed by reviewer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on a diff in pull request #4463: Remove prefix in jdbc interpreter
Posted by GitBox <gi...@apache.org>.
huage1994 commented on code in PR #4463:
URL: https://github.com/apache/zeppelin/pull/4463#discussion_r967616600
##########
jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java:
##########
@@ -31,49 +26,33 @@
import org.apache.hadoop.security.UserGroupInformation;
import org.apache.hadoop.security.alias.CredentialProvider;
import org.apache.hadoop.security.alias.CredentialProviderFactory;
-import org.apache.zeppelin.interpreter.SingleRowInterpreterResult;
-import org.apache.zeppelin.interpreter.ZeppelinContext;
+import org.apache.zeppelin.interpreter.*;
+import org.apache.zeppelin.interpreter.InterpreterResult.Code;
+import org.apache.zeppelin.interpreter.thrift.InterpreterCompletion;
import org.apache.zeppelin.interpreter.util.SqlSplitter;
import org.apache.zeppelin.jdbc.hive.HiveUtils;
+import org.apache.zeppelin.jdbc.security.JDBCSecurityImpl;
+import org.apache.zeppelin.scheduler.Scheduler;
+import org.apache.zeppelin.scheduler.SchedulerFactory;
import org.apache.zeppelin.tabledata.TableDataUtils;
+import org.apache.zeppelin.user.UserCredentials;
+import org.apache.zeppelin.user.UsernamePassword;
import org.apache.zeppelin.util.PropertiesUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.security.PrivilegedExceptionAction;
-import java.sql.Connection;
-import java.sql.DriverManager;
-import java.sql.ResultSet;
-import java.sql.ResultSetMetaData;
-import java.sql.SQLException;
-import java.sql.Statement;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Properties;
-import java.util.Set;
+import java.sql.*;
Review Comment:
> Please change your IDE settings not to use *.
Hi @jongyoul , thanks a lot for your advice! I'll do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4463: [ZEPPELIN-5493] Remove prefix in jdbc interpreter
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4463:
URL: https://github.com/apache/zeppelin/pull/4463#discussion_r983479720
##########
jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java:
##########
@@ -550,9 +549,9 @@ public Connection getConnection(String dbPrefix, InterpreterContext context)
LOGGER.debug("createSecureConfiguration() returned");
boolean isProxyEnabled = Boolean.parseBoolean(
getProperty("zeppelin.jdbc.auth.kerberos.proxy.enable", "true"));
- if (basePropertiesMap.get(dbPrefix).containsKey("proxy.user.property")
+ if (basePropertiesMap.get(DEFAULT_KEY).containsKey("proxy.user.property")
Review Comment:
Ref.
https://github.com/apache/zeppelin/blob/master/jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java#L216-L225
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4463: [ZEPPELIN-5493] Remove prefix in jdbc interpreter
Posted by GitBox <gi...@apache.org>.
huage1994 commented on PR #4463:
URL: https://github.com/apache/zeppelin/pull/4463#issuecomment-1260759390
> > BTW, don't it have a doc as well?
>
> Thanks @jongyoul for reminding! I found the [hive related doc](https://zeppelin.apache.org/docs/0.10.1/interpreter/jdbc.html#apache-hive) still have usage of this feature. I'll update it and verify.
Finished.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4463: [ZEPPELIN-5493] Remove prefix in jdbc interpreter
Posted by GitBox <gi...@apache.org>.
zjffdu commented on PR #4463:
URL: https://github.com/apache/zeppelin/pull/4463#issuecomment-1289875755
LGTM, will merge if no more comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on pull request #4463: [ZEPPELIN-5493] Remove prefix in jdbc interpreter
Posted by GitBox <gi...@apache.org>.
jongyoul commented on PR #4463:
URL: https://github.com/apache/zeppelin/pull/4463#issuecomment-1246278555
BTW, don't it have a doc as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4463: [ZEPPELIN-5493] Remove prefix in jdbc interpreter
Posted by GitBox <gi...@apache.org>.
huage1994 commented on PR #4463:
URL: https://github.com/apache/zeppelin/pull/4463#issuecomment-1246482194
> BTW, don't it have a doc as well?
Thanks @jongyoul for reminding!
I found the [hive related doc](https://zeppelin.apache.org/docs/0.10.1/interpreter/jdbc.html#apache-hive) still have usage of this feature. I'll update it and verify.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu merged pull request #4463: [ZEPPELIN-5493] Remove prefix in jdbc interpreter
Posted by GitBox <gi...@apache.org>.
zjffdu merged PR #4463:
URL: https://github.com/apache/zeppelin/pull/4463
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on a diff in pull request #4463: [ZEPPELIN-5493] Remove prefix in jdbc interpreter
Posted by GitBox <gi...@apache.org>.
huage1994 commented on code in PR #4463:
URL: https://github.com/apache/zeppelin/pull/4463#discussion_r985214869
##########
jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java:
##########
@@ -550,9 +549,9 @@ public Connection getConnection(String dbPrefix, InterpreterContext context)
LOGGER.debug("createSecureConfiguration() returned");
boolean isProxyEnabled = Boolean.parseBoolean(
getProperty("zeppelin.jdbc.auth.kerberos.proxy.enable", "true"));
- if (basePropertiesMap.get(dbPrefix).containsKey("proxy.user.property")
+ if (basePropertiesMap.get(DEFAULT_KEY).containsKey("proxy.user.property")
Review Comment:
> BTW, can we change `basePropertiesMap` not to use DEFAULT_KEY as a key?
Hi @jongyoul ,I totally agree with you.
I plan to refactor `JDBC` module in the very soon future.
As I was developing, I realized that removing `basePropertiesMap` would require a significant code change. I don't want this PR to get too big , so I plan to refactor `JDBC` module in another PR and do more verify.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on a diff in pull request #4463: [ZEPPELIN-5493] Remove prefix in jdbc interpreter
Posted by GitBox <gi...@apache.org>.
huage1994 commented on code in PR #4463:
URL: https://github.com/apache/zeppelin/pull/4463#discussion_r985214869
##########
jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java:
##########
@@ -550,9 +549,9 @@ public Connection getConnection(String dbPrefix, InterpreterContext context)
LOGGER.debug("createSecureConfiguration() returned");
boolean isProxyEnabled = Boolean.parseBoolean(
getProperty("zeppelin.jdbc.auth.kerberos.proxy.enable", "true"));
- if (basePropertiesMap.get(dbPrefix).containsKey("proxy.user.property")
+ if (basePropertiesMap.get(DEFAULT_KEY).containsKey("proxy.user.property")
Review Comment:
> BTW, can we change `basePropertiesMap` not to use DEFAULT_KEY as a key?
Hi @jongyoul ,I totally agree with you.
I plan to refactor `JDBC` module in the very soon future.
As I was developing, I realized that removing `basePropertiesMap` would require a significant code change. I don't want this PR to get too big , so I plan to refactor `JDBC` module in another PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4463: Remove prefix in jdbc interpreter
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4463:
URL: https://github.com/apache/zeppelin/pull/4463#discussion_r966977403
##########
jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java:
##########
@@ -31,49 +26,33 @@
import org.apache.hadoop.security.UserGroupInformation;
import org.apache.hadoop.security.alias.CredentialProvider;
import org.apache.hadoop.security.alias.CredentialProviderFactory;
-import org.apache.zeppelin.interpreter.SingleRowInterpreterResult;
-import org.apache.zeppelin.interpreter.ZeppelinContext;
+import org.apache.zeppelin.interpreter.*;
+import org.apache.zeppelin.interpreter.InterpreterResult.Code;
+import org.apache.zeppelin.interpreter.thrift.InterpreterCompletion;
import org.apache.zeppelin.interpreter.util.SqlSplitter;
import org.apache.zeppelin.jdbc.hive.HiveUtils;
+import org.apache.zeppelin.jdbc.security.JDBCSecurityImpl;
+import org.apache.zeppelin.scheduler.Scheduler;
+import org.apache.zeppelin.scheduler.SchedulerFactory;
import org.apache.zeppelin.tabledata.TableDataUtils;
+import org.apache.zeppelin.user.UserCredentials;
+import org.apache.zeppelin.user.UsernamePassword;
import org.apache.zeppelin.util.PropertiesUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.security.PrivilegedExceptionAction;
-import java.sql.Connection;
-import java.sql.DriverManager;
-import java.sql.ResultSet;
-import java.sql.ResultSetMetaData;
-import java.sql.SQLException;
-import java.sql.Statement;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Properties;
-import java.util.Set;
+import java.sql.*;
Review Comment:
Please change your IDE settings not to use *.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org