Skip to content

[KYUUBI #4111] JDBC ExecuteStatement operation should contain operationLog#4113

Closed
yikf wants to merge 1 commit intoapache:masterfrom
yikf:KYUUBI-4111
Closed

[KYUUBI #4111] JDBC ExecuteStatement operation should contain operationLog#4113
yikf wants to merge 1 commit intoapache:masterfrom
yikf:KYUUBI-4111

Conversation

@yikf
Copy link
Copy Markdown
Contributor

@yikf yikf commented Jan 6, 2023

Why are the changes needed?

Close #4111, JDBC ExecuteStatement operation should contain operationLog

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

@yikf
Copy link
Copy Markdown
Contributor Author

yikf commented Jan 6, 2023

cc @zhaomin1423

Copy link
Copy Markdown
Member

@zhaomin1423 zhaomin1423 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread kyuubi-common/src/main/scala/org/apache/kyuubi/operation/OperationManager.scala Outdated
Comment thread kyuubi-common/src/main/scala/org/apache/kyuubi/operation/OperationManager.scala Outdated
@yaooqinn
Copy link
Copy Markdown
Member

yaooqinn commented Jan 9, 2023

Hmm, isn't the error handled when an operation log is created?

@yaooqinn
Copy link
Copy Markdown
Member

yaooqinn commented Jan 9, 2023

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

@zhaomin1423
Copy link
Copy Markdown
Member

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.

@yikf
Copy link
Copy Markdown
Contributor Author

yikf commented Jan 9, 2023

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?

@yaooqinn
Copy link
Copy Markdown
Member

yaooqinn commented Jan 9, 2023

After this change, we can not determine whether the log is failed or just not implemented

@yaooqinn
Copy link
Copy Markdown
Member

yaooqinn commented Jan 9, 2023

2 can't work well now.

This is by contract, these OperationResouce's clients shall be able to benefit from the current implementation too.

@yikf
Copy link
Copy Markdown
Contributor Author

yikf commented Jan 9, 2023

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 ) : )

@yaooqinn
Copy link
Copy Markdown
Member

yaooqinn commented Jan 9, 2023

What are the benefits? (sorry for I didn't know the previous design ) : )

The benefits are what I described above for distinguishing errors with unimplemented.

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

@yikf yikf changed the title [KYUUBI #4111] Operations without log fetch log should fetch empty instead of report an error [KYUUBI #4111] JDBC ExecuteStatement operation should contain operationLog Jan 9, 2023
@yikf
Copy link
Copy Markdown
Contributor Author

yikf commented Jan 10, 2023

I fix this issue at jdbc engine side, please take a look if you find a moment, thanks @yaooqinn @turboFei @zhaomin1423

@turboFei turboFei closed this in 679810f Jan 10, 2023
@turboFei
Copy link
Copy Markdown
Member

thanks, merged to master

@turboFei turboFei added this to the v1.7.0 milestone Jan 10, 2023
@yikf yikf deleted the KYUUBI-4111 branch January 10, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] An exception encounter "failed to generate operation log" if operation don't implement getOperationLog

4 participants