You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by GitBox <gi...@apache.org> on 2023/01/06 13:00:19 UTC
[GitHub] [kyuubi] Yikf opened a new pull request, #4113: [KYUUBI #4111] Operations without log fetch log should fetch empty instead of report an error
Yikf opened a new pull request, #4113:
URL: https://github.com/apache/kyuubi/pull/4113
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/CONTRIBUTING.html
2. If the PR is related to an issue in https://github.com/apache/kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
Close https://github.com/apache/kyuubi/issues/4111, operations without log fetch log should fetch empty instead of report an error
### _How was this patch tested?_
- [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [x] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull 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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] Yikf commented on pull request #4113: [KYUUBI #4111] JDBC ExecuteStatement operation should contain operationLog
Posted by GitBox <gi...@apache.org>.
Yikf commented on PR #4113:
URL: https://github.com/apache/kyuubi/pull/4113#issuecomment-1376635623
I fix this issue at jdbc engine side, please take a look if you find a moment, thanks @yaooqinn @turboFei @zhaomin1423
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] turboFei commented on a diff in pull request #4113: [KYUUBI #4111] Operations without log fetch log should fetch empty instead of report an error
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #4113:
URL: https://github.com/apache/kyuubi/pull/4113#discussion_r1064114518
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/operation/OperationManager.scala:
##########
@@ -142,8 +143,12 @@ abstract class OperationManager(name: String) extends AbstractService(name) {
order: FetchOrientation,
maxRows: Int): TRowSet = {
val operationLog = getOperation(opHandle).getOperationLog
- operationLog.map(_.read(maxRows)).getOrElse {
- throw KyuubiSQLException(s"$opHandle failed to generate operation log")
+
Review Comment:
nit: not needed line
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] Yikf commented on pull request #4113: [KYUUBI #4111] Operations without log fetch log should fetch empty instead of report an error
Posted by GitBox <gi...@apache.org>.
Yikf commented on PR #4113:
URL: https://github.com/apache/kyuubi/pull/4113#issuecomment-1375340739
> After this change, we can not determine whether the log is failed or just not implemented
Good point, Have a question is which of the following designs is more reasonable:
1. Fetch empty log if the operation don't implements getOperationLog
2. Have the contract, client will only call fetching log for those operations which implement getOperationLog
If 1, we can handle whether the log is failed or just not implemented;
if 2, we can handle this case according to this contract at client/jdbc engine side;
> shall be able to benefit from the current implementation too.
What are the benefits? (sorry for I didn't know the previous design ) : )
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] turboFei commented on a diff in pull request #4113: [KYUUBI #4111] Operations without log fetch log should fetch empty instead of report an error
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #4113:
URL: https://github.com/apache/kyuubi/pull/4113#discussion_r1064114616
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/operation/OperationManager.scala:
##########
@@ -142,8 +143,12 @@ abstract class OperationManager(name: String) extends AbstractService(name) {
order: FetchOrientation,
maxRows: Int): TRowSet = {
val operationLog = getOperation(opHandle).getOperationLog
- operationLog.map(_.read(maxRows)).getOrElse {
- throw KyuubiSQLException(s"$opHandle failed to generate operation log")
+
+ operationLog match {
+ case Some(_) => operationLog.map(_.read(maxRows)).getOrElse {
Review Comment:
```
case Some(opLog) => opLog.read(maxRows)
```
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] zhaomin1423 commented on pull request #4113: [KYUUBI #4111] Operations without log fetch log should fetch empty instead of report an error
Posted by GitBox <gi...@apache.org>.
zhaomin1423 commented on PR #4113:
URL: https://github.com/apache/kyuubi/pull/4113#issuecomment-1375134119
> the original intentions are:
>
> 1. the operations that need operation logs shall implement getOperationLog
> 2. the client will only call fetching log for those operations which implement getOperationLog
> 3. the error raise only when the operation implements getOperationLog but fails to create the operation log file
2 can't work well now.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yaooqinn commented on pull request #4113: [KYUUBI #4111] Operations without log fetch log should fetch empty instead of report an error
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #4113:
URL: https://github.com/apache/kyuubi/pull/4113#issuecomment-1375123939
Hmm, isn't the error handled when an operation log is created?
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] turboFei commented on pull request #4113: [KYUUBI #4111] JDBC ExecuteStatement operation should contain operationLog
Posted by GitBox <gi...@apache.org>.
turboFei commented on PR #4113:
URL: https://github.com/apache/kyuubi/pull/4113#issuecomment-1377091775
thanks, merged to master
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] Yikf commented on pull request #4113: [KYUUBI #4111] Operations without log fetch log should fetch empty instead of report an error
Posted by GitBox <gi...@apache.org>.
Yikf commented on PR #4113:
URL: https://github.com/apache/kyuubi/pull/4113#issuecomment-1375146391
Yeah, 2 can not work well；
End-user can fetch logs through `OperationResource` as well, The client will only can fetch log for those operations which implement `getOperationLog` seems impossible to limit?
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yaooqinn commented on pull request #4113: [KYUUBI #4111] Operations without log fetch log should fetch empty instead of report an error
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #4113:
URL: https://github.com/apache/kyuubi/pull/4113#issuecomment-1375209773
After this change, we can not determine whether the log is failed or just not implemented
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] turboFei commented on a diff in pull request #4113: [KYUUBI #4111] Operations without log fetch log should fetch empty instead of report an error
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #4113:
URL: https://github.com/apache/kyuubi/pull/4113#discussion_r1064114616
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/operation/OperationManager.scala:
##########
@@ -142,8 +143,12 @@ abstract class OperationManager(name: String) extends AbstractService(name) {
order: FetchOrientation,
maxRows: Int): TRowSet = {
val operationLog = getOperation(opHandle).getOperationLog
- operationLog.map(_.read(maxRows)).getOrElse {
- throw KyuubiSQLException(s"$opHandle failed to generate operation log")
+
+ operationLog match {
+ case Some(_) => operationLog.map(_.read(maxRows)).getOrElse {
Review Comment:
```
case Some(log) => log.read(maxRows)
```
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yaooqinn commented on pull request #4113: [KYUUBI #4111] Operations without log fetch log should fetch empty instead of report an error
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #4113:
URL: https://github.com/apache/kyuubi/pull/4113#issuecomment-1375130267
the original intentions are:
1. the operations that need operation logs shall implement getOperationLog
2. the client will only call fetching log for those operations which implement getOperationLog
3. the error raise only when the operation implements getOperationLog but fails to create the operation log 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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yaooqinn commented on pull request #4113: [KYUUBI #4111] Operations without log fetch log should fetch empty instead of report an error
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #4113:
URL: https://github.com/apache/kyuubi/pull/4113#issuecomment-1375212029
> 2 can't work well now.
**This is by contract**, these OperationResouce's clients shall be able to benefit from the current implementation too.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] Yikf commented on pull request #4113: [KYUUBI #4111] Operations without log fetch log should fetch empty instead of report an error
Posted by GitBox <gi...@apache.org>.
Yikf commented on PR #4113:
URL: https://github.com/apache/kyuubi/pull/4113#issuecomment-1373591917
cc @zhaomin1423
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yaooqinn commented on pull request #4113: [KYUUBI #4111] Operations without log fetch log should fetch empty instead of report an error
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #4113:
URL: https://github.com/apache/kyuubi/pull/4113#issuecomment-1375397255
> What are the benefits? (sorry for I didn't know the previous design ) : )
Well, the thrift and rest calls have no differences on both the log API and the backend operations. It's up to much higher level APIs(such as JDBC), or end-users, to do the error handling.
Currently, the meta operations are no operation logs and are executed synchronously. The rest call can also invoke these operations synchronously, which makes it has no chance to call log fetching
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] turboFei closed pull request #4113: [KYUUBI #4111] JDBC ExecuteStatement operation should contain operationLog
Posted by GitBox <gi...@apache.org>.
turboFei closed pull request #4113: [KYUUBI #4111] JDBC ExecuteStatement operation should contain operationLog
URL: https://github.com/apache/kyuubi/pull/4113
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org