You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/08/27 08:49:35 UTC
[GitHub] [calcite] fageiguanbing opened a new pull request #2123: Support for Elasticsearch basic authentication
fageiguanbing opened a new pull request #2123:
URL: https://github.com/apache/calcite/pull/2123
[JIRA](https://issues.apache.org/jira/browse/CALCITE-4180#)
I found that there isn't any implementation of Elascticsearch authentication. So I fix it. Please review my code.
I get inspiration from [here](https://calcite.apache.org/docs/adapter.html#drivers), So I use `jdbcUser / jdbcPassword`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
chunweilei commented on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684157755
Please correct the format of the commit message.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] michaelmior commented on pull request #2123: [CALCITE-4180] Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
michaelmior commented on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684648703
> I would slide to jdbcXXX if use connect to the system with a JDBC connection and where the parameters are involved in it, or there would be many specific parameters there: esXXX, mongoDBXXX ..
The names of the parameters however are independent of how the connection is made. Also, they are already specific to the adapter used so there is no need for a prefix. Other adapters such as those for Cassandra and MongoDB just use `username` and `password` already.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] fageiguanbing removed a comment on pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
fageiguanbing removed a comment on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684159651
> @amaliujia You are correct although JDBC isn't the only way to use Calcite. In any case, my main concern is that here is that the parameters are for the Elasticsearch username and password, not the parameters for any JDBC connection.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] fageiguanbing commented on pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
fageiguanbing commented on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684164082
> Please correct the format of the commit message.
what should I write commit messages?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#discussion_r478834587
##########
File path: elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/ElasticsearchSchemaFactory.java
##########
@@ -100,14 +106,27 @@ public ElasticsearchSchemaFactory() {
/**
* Builds elastic rest client from user configuration
* @param hosts list of ES HTTP Hosts to connect to
+ * @param jdbcUser the username of ES
+ * @param jdbcPassword the password of ES
* @return newly initialized low-level rest http client for ES
*/
- private static RestClient connect(List<HttpHost> hosts, String pathPrefix) {
+ private static RestClient connect(List<HttpHost> hosts, String pathPrefix, String jdbcUser,
+ String jdbcPassword) {
Review comment:
Fix the indentation. Is it easy to add a test case there ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] fageiguanbing commented on pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
fageiguanbing commented on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684159651
> @amaliujia You are correct although JDBC isn't the only way to use Calcite. In any case, my main concern is that here is that the parameters are for the Elasticsearch username and password, not the parameters for any JDBC connection.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] michaelmior commented on pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
michaelmior commented on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684131685
@amaliujia You are correct although JDBC isn't the only way to use Calcite. In any case, my main concern is that here is that the parameters are for the Elasticsearch username and password, not the parameters for any JDBC connection.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
danny0405 commented on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684178607
> @amaliujia You are correct although JDBC isn't the only way to use Calcite. In any case, my main concern is that here is that the parameters are for the Elasticsearch username and password, not the parameters for any JDBC connection.
I would slide to jdbcXXX if use connect to the system with a JDBC connection and where the parameters are involved in it, or there would be many specific parameters there: esXXX, mongoDBXXX ..
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] fageiguanbing commented on a change in pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
fageiguanbing commented on a change in pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#discussion_r478879223
##########
File path: elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/ElasticsearchSchemaFactory.java
##########
@@ -100,14 +106,27 @@ public ElasticsearchSchemaFactory() {
/**
* Builds elastic rest client from user configuration
* @param hosts list of ES HTTP Hosts to connect to
+ * @param jdbcUser the username of ES
+ * @param jdbcPassword the password of ES
* @return newly initialized low-level rest http client for ES
*/
- private static RestClient connect(List<HttpHost> hosts, String pathPrefix) {
+ private static RestClient connect(List<HttpHost> hosts, String pathPrefix, String jdbcUser,
+ String jdbcPassword) {
Review comment:
I don't know where to find the specification. Could you tell me where it is?
I guess:
```java
private static RestClient connect(List<HttpHost> hosts, String pathPrefix,
String jdbcUser, String jdbcPassword) {
```
I add a new class in test package. And I think I should add `@Disabled` to prevent it execute in build.
```java
package org.apache.calcite.adapter.elasticsearch;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import static org.junit.jupiter.api.Assertions.assertEquals;
/**
* Testing Elasticsearch Authentication.
*/
@Disabled("need a elasticsearch cluster")
public class AuthenticationTest {
/**
* You need specify jdbcUser/jdbcPassword in elasticsearch.json
*/
@Test void select1() throws ClassNotFoundException, SQLException {
Class.forName("org.apache.calcite.jdbc.Driver");
try (Connection con = DriverManager
.getConnection("jdbc:calcite:model=src/test/resources/elasticsearch.json")) {
try (Statement stmt = con.createStatement()) {
String sql = "select 1";
try (ResultSet rs = stmt.executeQuery(sql)) {
if (rs.next()) {
assertEquals(rs.getObject(1), 1);
}
}
}
}
}
}
```
```json
{
"version": "1.0",
"defaultSchema": "elasticsearch",
"schemas": [
{
"type": "custom",
"name": "elasticsearch",
"factory": "org.apache.calcite.adapter.elasticsearch.ElasticsearchSchemaFactory",
"operand": {
"jdbcUser": "elastic",
"jdbcPassword": "elastic",
"coordinates": "{'127.0.0.1': 9200}"
}
}
]
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684079267
@michaelmior
From [adapter.html#drivers](https://calcite.apache.org/docs/adapter.html#drivers) I am think it says can use `jdbcuser` and `jdbcpassword` to to connect systems by JDBC approach in Calcite. So I guess this PR is doing right. Am I mis-understanding something?
@fageiguanbing
Can you update your JIRA title with `[CALCITE-xxxx] title` format?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] fageiguanbing closed pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
fageiguanbing closed pull request #2123:
URL: https://github.com/apache/calcite/pull/2123
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on pull request #2123: [CALCITE-4180] Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
danny0405 commented on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684671680
> > I would slide to jdbcXXX if use connect to the system with a JDBC connection and where the parameters are involved in it, or there would be many specific parameters there: esXXX, mongoDBXXX ..
>
> The names of the parameters however are independent of how the connection is made. Also, they are already specific to the adapter used so there is no need for a prefix. Other adapters such as those for Cassandra and MongoDB just use `username` and `password` already.
Yeah, we already 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684165424
The commit message usually can be
```
[CALCITE-xxxx] commit message (your name)
Context/examples if you want to add any. Usually extra context is highly
recommended for complicated changes.
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia edited a comment on pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684079267
@michaelmior
From [adapter.html#drivers](https://calcite.apache.org/docs/adapter.html#drivers) I am thinking it says one can use `jdbcuser` and `jdbcpassword` to connect systems by JDBC approach in Calcite. So I guess this PR is doing right. Am I mis-understanding something?
@fageiguanbing
Can you update your PR title with `[CALCITE-xxxx] title` format?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] fageiguanbing commented on pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
fageiguanbing commented on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684164299
> > Please correct the format of the commit message.
>
> what should I write commit messages?
@chunweilei Hi, what should I write commit messages?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
chunweilei commented on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684167359
> > > Please correct the format of the commit message.
> >
> >
> > what should I write commit messages?
>
> @chunweilei Hi, what should I write commit messages?
You can refer to the doc[1].
[1]https://calcite.apache.org/develop/#contributing
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] michaelmior commented on a change in pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
michaelmior commented on a change in pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#discussion_r480293259
##########
File path: elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/ElasticsearchSchemaFactory.java
##########
@@ -88,7 +92,9 @@ public ElasticsearchSchemaFactory() {
}
final String pathPrefix = (String) map.get("pathPrefix");
// create client
- final RestClient client = connect(hosts, pathPrefix);
+ String jdbcUser = (String) map.get("jdbcUser");
+ String jdbcPassword = (String) map.get("jdbcPassword");
+ final RestClient client = connect(hosts, pathPrefix, jdbcUser, jdbcPassword);
Review comment:
Given that this refers to the username and password for Elasticsearch and is not directly related to JDBC, this should be changed to `username` and `password`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-683556162
+1 Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] fageiguanbing removed a comment on pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
fageiguanbing removed a comment on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684164082
> Please correct the format of the commit message.
what should I write commit messages?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia edited a comment on pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684079267
@michaelmior
From [adapter.html#drivers](https://calcite.apache.org/docs/adapter.html#drivers) I am thinking it says one can use `jdbcuser` and `jdbcpassword` to connect systems by JDBC approach in Calcite. So I guess this PR is doing right. Am I mis-understanding something?
@fageiguanbing
Can you update your JIRA title with `[CALCITE-xxxx] title` format?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#discussion_r478834432
##########
File path: elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/ElasticsearchSchemaFactory.java
##########
@@ -100,14 +106,27 @@ public ElasticsearchSchemaFactory() {
/**
* Builds elastic rest client from user configuration
* @param hosts list of ES HTTP Hosts to connect to
+ * @param jdbcUser the username of ES
+ * @param jdbcPassword the password of ES
* @return newly initialized low-level rest http client for ES
*/
- private static RestClient connect(List<HttpHost> hosts, String pathPrefix) {
+ private static RestClient connect(List<HttpHost> hosts, String pathPrefix, String jdbcUser,
+ String jdbcPassword) {
Objects.requireNonNull(hosts, "hosts or coordinates");
Preconditions.checkArgument(!hosts.isEmpty(), "no ES hosts specified");
RestClientBuilder builder = RestClient.builder(hosts.toArray(new HttpHost[hosts.size()]));
+
+ if (jdbcUser != null && !jdbcUser.isEmpty()
+ && jdbcPassword != null && !jdbcPassword.isEmpty()) {
Review comment:
you can use `Strings.isNullOrEmpty()`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] fageiguanbing commented on pull request #2123: [CALCITE-4180] Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
fageiguanbing commented on pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#issuecomment-684790596
@michaelmior @danny0405 @amaliujia thanks for all
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] fageiguanbing commented on a change in pull request #2123: Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
fageiguanbing commented on a change in pull request #2123:
URL: https://github.com/apache/calcite/pull/2123#discussion_r478879223
##########
File path: elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/ElasticsearchSchemaFactory.java
##########
@@ -100,14 +106,27 @@ public ElasticsearchSchemaFactory() {
/**
* Builds elastic rest client from user configuration
* @param hosts list of ES HTTP Hosts to connect to
+ * @param jdbcUser the username of ES
+ * @param jdbcPassword the password of ES
* @return newly initialized low-level rest http client for ES
*/
- private static RestClient connect(List<HttpHost> hosts, String pathPrefix) {
+ private static RestClient connect(List<HttpHost> hosts, String pathPrefix, String jdbcUser,
+ String jdbcPassword) {
Review comment:
I don't know where to find the specification. Could you tell me where it is?
I guess:
```java
private static RestClient connect(List<HttpHost> hosts, String pathPrefix,
String jdbcUser, String jdbcPassword) {
```
I add a new class in test package. And I think I should add `@Disabled` to prevent it execute in build.
```java
package org.apache.calcite.adapter.elasticsearch;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import static org.junit.jupiter.api.Assertions.assertEquals;
/**
* Testing Elasticsearch Authentication.
*/
@Disabled("need a elasticsearch cluster")
public class AuthenticationTest {
/**
* You need specify jdbcUser/jdbcPassword in elasticsearch.json
*/
@Test void select1() throws ClassNotFoundException, SQLException {
Class.forName("org.apache.calcite.jdbc.Driver");
try (Connection con = DriverManager
.getConnection("jdbc:calcite:model=src/test/resources/elasticsearch.json")) {
try (Statement stmt = con.createStatement()) {
String sql = "select 1";
try (ResultSet rs = stmt.executeQuery(sql)) {
if (rs.next()) {
assertEquals(rs.getObject(1), 1);
}
}
}
}
}
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 closed pull request #2123: [CALCITE-4180] Support for Elasticsearch basic authentication
Posted by GitBox <gi...@apache.org>.
danny0405 closed pull request #2123:
URL: https://github.com/apache/calcite/pull/2123
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org