You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tokoko (via GitHub)" <gi...@apache.org> on 2023/04/02 01:26:05 UTC
[GitHub] [arrow-adbc] tokoko opened a new pull request, #572: feat(java/driver/flight-sql): add basic auth
tokoko opened a new pull request, #572:
URL: https://github.com/apache/arrow-adbc/pull/572
@lidavidm I want a quick feedback on some of the changes.
- Removed unused FlightClient from `FlightSqlDatabase`
- FlightClient creation moved from `FlightSqlDatabase` to `FlightSqlConnection`. In the existing version after connect() is called a new client object is created, but all of them shared the same allocator. That was probably a bug, right?
- username/password passed as parameters named `adbc.username` and `adbc.password` (to be added in `AdbcDriver`). Are there any plans to have some sort of connection string syntax like JDBC?
- As there can be more than one FlightClients, I wanted to avoid keeping track of auth token, so added a middleware that does both: extract an auth token and pass it on future calls. It's functionally similar to the go example, but I couldn't use `authenticateBasicToken` because I couldn't find a way to extract bearer value from CredenialCallOption. Maybe there's a simpler way to accomplish this???
- I'd like to keep FlightClient creation logic contained in `clientCache`. Right now the main FlightClient is created independently and only Endpoint clients are obtained from cache. If changed, main client will always have to be obtained from cache before usage, otherwise it might have expired.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #572: feat(java/driver/flight-sql): add basic auth
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #572:
URL: https://github.com/apache/arrow-adbc/pull/572#discussion_r1174917382
##########
java/driver/flight-sql/src/main/java/org/apache/arrow/adbc/driver/flightsql/FlightSqlConnection.java:
##########
@@ -62,7 +74,51 @@ public class FlightSqlConnection implements AdbcConnection {
throw new RuntimeException(e);
}
})
- .build(location -> FlightClient.builder(allocator, location).build());
+ .build(
+ loc -> {
+ FlightClient flightClient =
+ FlightClient.builder(allocator, loc)
+ .intercept(
+ new FlightClientMiddleware.Factory() {
Review Comment:
That would be fine IMO. Once the code starts creeping too much to the right it gets a little hard to read.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(java/driver/flight-sql): add basic auth [arrow-adbc]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #572:
URL: https://github.com/apache/arrow-adbc/pull/572#issuecomment-1905066719
FYI the JDBC Flight SQL Driver implements optional sharing of the bearer token on spawned connections. It does so when building the driver -- it reads the token from the middle ware then on subsequent FlightClients appends it as a header parameter to the request.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(java/driver/flight-sql): add basic auth [arrow-adbc]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #572:
URL: https://github.com/apache/arrow-adbc/pull/572#issuecomment-1928550338
Completed in #1487
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #572: feat(java/driver/flight-sql): add basic auth
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #572:
URL: https://github.com/apache/arrow-adbc/pull/572#issuecomment-1494279972
Hey - it's gonna be a few weeks before I can review
just fyi, C/Go use just "username" and "password" so best to stick to that if possible
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(java/driver/flight-sql): add basic auth [arrow-adbc]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo closed pull request #572: feat(java/driver/flight-sql): add basic auth
URL: https://github.com/apache/arrow-adbc/pull/572
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #572: feat(java/driver/flight-sql): add basic auth
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #572:
URL: https://github.com/apache/arrow-adbc/pull/572#discussion_r1169349033
##########
java/driver/flight-sql-validation/src/test/java/org/apache/arrow/adbc/driver/flightsql/FlightSqlQuirks.java:
##########
@@ -48,6 +50,8 @@ public AdbcDatabase initDatabase(BufferAllocator allocator) throws AdbcException
final Map<String, Object> parameters = new HashMap<>();
parameters.put(AdbcDriver.PARAM_URL, url);
+ parameters.put("adbc.username", System.getenv(FLIGHT_SQL_USER_ENV_VAR));
Review Comment:
As mentioned, let's use just "username" for consistency with other implementations
##########
java/driver/flight-sql/src/main/java/org/apache/arrow/adbc/driver/flightsql/FlightSqlConnection.java:
##########
@@ -62,7 +74,51 @@ public class FlightSqlConnection implements AdbcConnection {
throw new RuntimeException(e);
}
})
- .build(location -> FlightClient.builder(allocator, location).build());
+ .build(
+ loc -> {
+ FlightClient flightClient =
+ FlightClient.builder(allocator, loc)
+ .intercept(
+ new FlightClientMiddleware.Factory() {
Review Comment:
nit: could we pull this out into a static inner class instead of having it inline here?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] tokoko commented on a diff in pull request #572: feat(java/driver/flight-sql): add basic auth
Posted by "tokoko (via GitHub)" <gi...@apache.org>.
tokoko commented on code in PR #572:
URL: https://github.com/apache/arrow-adbc/pull/572#discussion_r1174483988
##########
java/driver/flight-sql/src/main/java/org/apache/arrow/adbc/driver/flightsql/FlightSqlConnection.java:
##########
@@ -62,7 +74,51 @@ public class FlightSqlConnection implements AdbcConnection {
throw new RuntimeException(e);
}
})
- .build(location -> FlightClient.builder(allocator, location).build());
+ .build(
+ loc -> {
+ FlightClient flightClient =
+ FlightClient.builder(allocator, loc)
+ .intercept(
+ new FlightClientMiddleware.Factory() {
Review Comment:
That would make Factory a doubly nested inner class, probably best if I split it into an entirely separate file.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #572: feat(java/driver/flight-sql): add basic auth
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #572:
URL: https://github.com/apache/arrow-adbc/pull/572#issuecomment-1500249294
> In the existing version after connect() is called a new client object is created, but all of them shared the same allocator. That was probably a bug, right?
See some recent Java commits/issues that change up how allocators are handled (the current implementation had leaks): https://github.com/apache/arrow-adbc/commit/e69723a158276d973cf6b84127ea590844e1efb5, https://github.com/apache/arrow-adbc/issues/563, https://github.com/apache/arrow-adbc/pull/564
> username/password passed as parameters named adbc.username and adbc.password (to be added in AdbcDriver). Are there any plans to have some sort of connection string syntax like JDBC?
Only URIs for now (C/Go have settled on "uri" as the standard parameter, Java needs to align with that too)
> It's functionally similar to the go example, but I couldn't use authenticateBasicToken because I couldn't find a way to extract bearer value from CredenialCallOption. Maybe there's a simpler way to accomplish this???
I will have to dig later but possibly/probably upstream needs to be fixed first; I don't think any of that was implemented to really do anything other than support the JDBC driver specifically
> I'd like to keep FlightClient creation logic contained in clientCache. Right now the main FlightClient is created independently and only Endpoint clients are obtained from cache. If changed, main client will always have to be obtained from cache before usage, otherwise it might have expired.
It seems you could factor out a factory function that can be used to create a one-off main client as well as cached clients?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] aiguofer commented on a diff in pull request #572: feat(java/driver/flight-sql): add basic auth
Posted by "aiguofer (via GitHub)" <gi...@apache.org>.
aiguofer commented on code in PR #572:
URL: https://github.com/apache/arrow-adbc/pull/572#discussion_r1212559765
##########
java/driver/flight-sql/src/main/java/org/apache/arrow/adbc/driver/flightsql/FlightSqlDriver.java:
##########
@@ -64,6 +64,11 @@ public AdbcDatabase open(Map<String, Object> parameters) throws AdbcException {
} else {
quirks = new SqlQuirks();
}
- return new FlightSqlDatabase(allocator, location, (SqlQuirks) quirks);
+ return new FlightSqlDatabase(
+ allocator,
+ location,
+ (SqlQuirks) quirks,
+ (String) parameters.get("adbc.username"),
+ (String) parameters.get("adbc.password"));
Review Comment:
It might be better to pass the `parameters` instead. The `FlightSqlDatabase` will need to know about all the params to decide how to build the `FlightClient`. For example, all the TLS options.
It would also be nice to have dataclasses or static objects with all the option values https://arrow.apache.org/adbc/main/driver/go/flight_sql.html#client-options
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] tokoko commented on pull request #572: feat(java/driver/flight-sql): add basic auth
Posted by "tokoko (via GitHub)" <gi...@apache.org>.
tokoko commented on PR #572:
URL: https://github.com/apache/arrow-adbc/pull/572#issuecomment-1494322308
Okay, I'll push ahead with dremio suite then and you can review it at the end
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(java/driver/flight-sql): add basic auth [arrow-adbc]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #572:
URL: https://github.com/apache/arrow-adbc/pull/572#issuecomment-1904833699
@tokoko , this issue hasn't been updated since May. May I help finish getting this one merged?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org