You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "huangzhir (via GitHub)" <gi...@apache.org> on 2023/05/10 10:04:34 UTC
[GitHub] [kyuubi] huangzhir opened a new pull request, #4813: Add the operationHandle parameter to the /v1/operations/:operationId/log interface
huangzhir opened a new pull request, #4813:
URL: https://github.com/apache/kyuubi/pull/4813
<!--
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.
-->
to close https://github.com/apache/kyuubi/issues/4732
### _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
- [X] [Run test](https://kyuubi.readthedocs.io/en/master/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] ulysses-you commented on a diff in pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#discussion_r1217401576
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/operation/log/OperationLog.scala:
##########
@@ -172,6 +196,21 @@ class OperationLog(path: Path) {
toRowSet(logs)
}
+ private def resetReader(): Unit = {
+ lock.synchronized {
Review Comment:
does not `synchronized { ... }` have the same effect ? other methods are protected by `synchronized {...}`, then if we introduce a new object to lock, I'm afraid there would be deadlock issue.
--
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] huangzhir commented on pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "huangzhir (via GitHub)" <gi...@apache.org>.
huangzhir commented on PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#issuecomment-1545359175
@ulysses-you Thank you for your insightful comments. I concur with you on the significance of both FETCH_FIRST and FETCH_PRIOR. FETCH_FIRST is indeed crucial when it comes to re-reading logs. Regarding FETCH_PRIOR, although the performance may not be ideal at the moment, its usage cases may be relatively few. Therefore, I propose we retain it for now. If there arises a need in the future, we can look into optimizing it, possibly with your suggestion of using a RandomAccess File. Your feedback is highly appreciated.
--
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] ulysses-you commented on a diff in pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#discussion_r1212931337
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/OperationsResource.scala:
##########
@@ -140,11 +140,13 @@ private[v1] class OperationsResource extends ApiRequestContext with Logging {
@Path("{operationHandle}/log")
def getOperationLog(
@PathParam("operationHandle") operationHandleStr: String,
- @QueryParam("maxrows") maxRows: Int): OperationLog = {
+ @QueryParam("maxrows") @DefaultValue("100") maxRows: Int,
+ @QueryParam("fetchorientation") @DefaultValue("FETCH_NEXT")
+ fetchOrientation: String): OperationLog = {
try {
val rowSet = fe.be.sessionManager.operationManager.getOperationLogRowSet(
OperationHandle(operationHandleStr),
- FetchOrientation.FETCH_NEXT,
+ FetchOrientation.withName(fetchOrientation),
Review Comment:
can we fast fail there that throw exception if user requests `FETCH_PRIOR` ?
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/operation/log/OperationLog.scala:
##########
@@ -154,14 +167,25 @@ class OperationLog(path: Path) {
tRow
}
+ def read(maxRows: Int): TRowSet = synchronized {
+ read(FETCH_NEXT, maxRows)
+ }
+
/**
* Read to log file line by line
*
* @param maxRows maximum result number can reach
+ * @param order the fetch orientation of the result, can be FETCH_NEXT, FETCH_FIRST
*/
- def read(maxRows: Int): TRowSet = synchronized {
+ def read(order: FetchOrientation = FETCH_NEXT, maxRows: Int): TRowSet = synchronized {
if (!initialized) return ThriftUtils.newEmptyRowSet
- val (logs, lines) = readLogs(reader, maxRows, maxRows)
+ if (order != FETCH_NEXT && order != FETCH_FIRST) {
+ throw KyuubiSQLException(s"$order in operation log is not supported")
+ }
+ if (order == FETCH_FIRST) {
+ resetReader()
Review Comment:
how about just close seekableReader and create a new seekableReader?
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/operation/log/OperationLog.scala:
##########
@@ -172,6 +196,21 @@ class OperationLog(path: Path) {
toRowSet(logs)
}
+ private def resetReader(): Unit = {
+ lock.synchronized {
+ trySafely {
+ if (reader != null) {
+ reader.close()
+ }
+ }
+ reader = null
+ closeExtraReaders()
+ extraReaders.clear()
+ extraPaths.foreach(path =>
+ extraReaders += Files.newBufferedReader(path, StandardCharsets.UTF_8))
+ }
+ }
+
def read(from: Int, size: Int): TRowSet = synchronized {
Review Comment:
this is used for batch mode, we can make a new pr to support FETCH_FIRST.
--
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] huangzhir commented on a diff in pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "huangzhir (via GitHub)" <gi...@apache.org>.
huangzhir commented on code in PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#discussion_r1214002883
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/OperationsResource.scala:
##########
@@ -140,11 +140,13 @@ private[v1] class OperationsResource extends ApiRequestContext with Logging {
@Path("{operationHandle}/log")
def getOperationLog(
@PathParam("operationHandle") operationHandleStr: String,
- @QueryParam("maxrows") maxRows: Int): OperationLog = {
+ @QueryParam("maxrows") @DefaultValue("100") maxRows: Int,
+ @QueryParam("fetchorientation") @DefaultValue("FETCH_NEXT")
+ fetchOrientation: String): OperationLog = {
try {
val rowSet = fe.be.sessionManager.operationManager.getOperationLogRowSet(
OperationHandle(operationHandleStr),
- FetchOrientation.FETCH_NEXT,
+ FetchOrientation.withName(fetchOrientation),
Review Comment:
done
--
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] huangzhir commented on a diff in pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "huangzhir (via GitHub)" <gi...@apache.org>.
huangzhir commented on code in PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#discussion_r1214624426
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/operation/log/OperationLog.scala:
##########
@@ -172,6 +196,21 @@ class OperationLog(path: Path) {
toRowSet(logs)
}
+ private def resetReader(): Unit = {
+ lock.synchronized {
Review Comment:
I think that REST log requests can easily cause concurrent operations, so there is a high likelihood of concurrent access to resetReader(). If no lock is used, two scenarios may occur:
1. Data race: Multiple threads simultaneously operate on shared resources such as reader and extraReaders, which can lead to data inconsistency. For example, one thread may be closing the reader and setting it to null, while another thread might attempt to close the same reader, resulting in unexpected exceptions.
3. Inconsistent state: Due to the concurrent access of extraReaders by multiple threads, errors may occur during the execution of extraPaths.foreach(...). For instance, one thread may clear extraReaders after another thread has already created a new BufferedReader, resulting in an inconsistent state.
Therefore, I have decided to add a lock mechanism in that section.
--
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] codecov-commenter commented on pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#issuecomment-1570030561
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/4813?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#4813](https://app.codecov.io/gh/apache/kyuubi/pull/4813?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (162e008) into [master](https://app.codecov.io/gh/apache/kyuubi/commit/4c3c36e77f3b5cd0bb75339b7743fb1f2a7aa788?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (4c3c36e) will **not change** coverage.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #4813 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 557 557
Lines 30696 30725 +29
Branches 3994 4000 +6
======================================
- Misses 30696 30725 +29
```
| [Impacted Files](https://app.codecov.io/gh/apache/kyuubi/pull/4813?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...rk/sql/execution/arrow/KyuubiArrowConverters.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4813?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvc3Bhcmsvc3FsL2V4ZWN1dGlvbi9hcnJvdy9LeXV1YmlBcnJvd0NvbnZlcnRlcnMuc2NhbGE=) | `0.00% <ø> (ø)` | |
| [...org/apache/kyuubi/operation/OperationManager.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4813?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...org/apache/kyuubi/operation/log/OperationLog.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4813?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vbG9nL09wZXJhdGlvbkxvZy5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4813?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/server/api/v1/OperationsResource.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4813?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvYXBpL3YxL09wZXJhdGlvbnNSZXNvdXJjZS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
... and [2 files with indirect coverage changes](https://app.codecov.io/gh/apache/kyuubi/pull/4813/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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] ulysses-you commented on pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#issuecomment-1577767444
thank you @huangzhir , merging 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] huangzhir commented on a diff in pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "huangzhir (via GitHub)" <gi...@apache.org>.
huangzhir commented on code in PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#discussion_r1213889339
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/operation/log/OperationLog.scala:
##########
@@ -154,14 +167,25 @@ class OperationLog(path: Path) {
tRow
}
+ def read(maxRows: Int): TRowSet = synchronized {
+ read(FETCH_NEXT, maxRows)
+ }
+
/**
* Read to log file line by line
*
* @param maxRows maximum result number can reach
+ * @param order the fetch orientation of the result, can be FETCH_NEXT, FETCH_FIRST
*/
- def read(maxRows: Int): TRowSet = synchronized {
+ def read(order: FetchOrientation = FETCH_NEXT, maxRows: Int): TRowSet = synchronized {
if (!initialized) return ThriftUtils.newEmptyRowSet
- val (logs, lines) = readLogs(reader, maxRows, maxRows)
+ if (order != FETCH_NEXT && order != FETCH_FIRST) {
+ throw KyuubiSQLException(s"$order in operation log is not supported")
+ }
+ if (order == FETCH_FIRST) {
+ resetReader()
Review Comment:
I think just closing the ‘seekableReader’ and creating a new ‘seekableReader’ may not work because ‘seekableReader’ and ‘reader’ are different objects. Unless all instances of ‘reader’ are also replaced with ‘seekableReader’,
--
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] huangzhir commented on pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "huangzhir (via GitHub)" <gi...@apache.org>.
huangzhir commented on PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#issuecomment-1571408642
> @huangzhir thank you for the work. It seems the performance is poor for `FETCH_PRIOR`, for each request, we will reopen the log file and seek to taget position. Is there a real world use case for it ? If so, we might need to rewrite the code using `RandomAccessFile`. `FETCH_FIRST` looks more useful to me.
I looked at the Hive code (link: https://github.com/apache/hive/blob/a5d7f72dfe51fe8119d209bf61c64b8e94f2f42b/ql/src/java/org/apache/hadoop/hive/ql/session/OperationLog.java#LL156C31-L156C35) and noticed that FETCH_PRIOR is not implemented. Therefore, I have removed FETCH_PRIOR and referenced the Hive OperationLog code to rewrite the code.
@ulysses-you Can you please review it?"
--
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] ulysses-you commented on a diff in pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#discussion_r1214005214
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/operation/log/OperationLog.scala:
##########
@@ -172,6 +196,21 @@ class OperationLog(path: Path) {
toRowSet(logs)
}
+ private def resetReader(): Unit = {
+ lock.synchronized {
Review Comment:
why not just use `synchronized { ... }` ?
--
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] ulysses-you commented on a diff in pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#discussion_r1217909523
##########
kyuubi-common/src/test/scala/org/apache/kyuubi/log/SeekableBufferedReaderSuite.scala:
##########
@@ -94,6 +95,80 @@ class SeekableBufferedReaderSuite extends KyuubiFunSuite {
reader.close()
}
+ // borrow the test cases from FetchIteratorSuite
+ private def iteratorTest(fetchIter: FetchIterator[String]): Unit = {
+ fetchIter.fetchNext()
+ assert(fetchIter.getFetchStart == 0)
+ assert(fetchIter.getPosition == 0)
+ assertResult(0 until 2)(getRows(fetchIter, 2))
+ assert(fetchIter.getFetchStart == 0)
+ assert(fetchIter.getPosition == 2)
+
+ fetchIter.fetchNext()
+ assert(fetchIter.getFetchStart == 2)
+ assert(fetchIter.getPosition == 2)
+ assertResult(2 until 3)(getRows(fetchIter, 1))
+ assert(fetchIter.getFetchStart == 2)
+ assert(fetchIter.getPosition == 3)
+
+ fetchIter.fetchPrior(2)
Review Comment:
does the test relate to this pr ? it seems we do not touch `FetchIterator`.
--
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] huangzhir commented on a diff in pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "huangzhir (via GitHub)" <gi...@apache.org>.
huangzhir commented on code in PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#discussion_r1217524365
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/operation/log/OperationLog.scala:
##########
@@ -172,6 +196,21 @@ class OperationLog(path: Path) {
toRowSet(logs)
}
+ private def resetReader(): Unit = {
+ lock.synchronized {
+ trySafely {
+ if (reader != null) {
+ reader.close()
+ }
+ }
+ reader = null
+ closeExtraReaders()
+ extraReaders.clear()
+ extraPaths.foreach(path =>
+ extraReaders += Files.newBufferedReader(path, StandardCharsets.UTF_8))
+ }
+ }
+
def read(from: Int, size: Int): TRowSet = synchronized {
Review Comment:
done
--
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] huangzhir commented on pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "huangzhir (via GitHub)" <gi...@apache.org>.
huangzhir commented on PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#issuecomment-1543571356
The original OperationLog class had a read(maxRows: Int) method without the FetchOrientation parameter, supporting only the FETCH_NEXT mode for reading data. However, when retrieving logs from the AbstractSession, the result data has the FetchOrientation parameter.
``` scala
override def fetchResults(
operationHandle: OperationHandle,
orientation: FetchOrientation,
maxRows: Int,
fetchLog: Boolean): TRowSet = {
if (fetchLog) {
sessionManager.operationManager.getOperationLogRowSet(operationHandle, orientation, maxRows)
} else {
sessionManager.operationManager.getOperationNextRowSet(operationHandle, orientation, maxRows)
}
}
```
In the original OperationManager code, due to the lack of FetchOrientation support in the OperationLog's read method, the implementation ignored the FetchOrientation parameter.
```scala
def getOperationLogRowSet(
opHandle: OperationHandle,
order: FetchOrientation,
maxRows: Int): TRowSet = {
val operationLog = getOperation(opHandle).getOperationLog
operationLog.map(_.read(maxRows)).getOrElse {
throw KyuubiSQLException(s"$opHandle failed to generate operation log")
}
}
```
As a result, the operation logs provided to the external interface do not support the FetchOrientation parameter, and it is not possible to review the log data for an operation.
The mentioned pull request adds the FetchOrientation parameter to the OperationLog.read method and introduces support for the FetchOrientation parameter in the /v1/operations/:operationId/log interface.
--
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] ulysses-you commented on pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#issuecomment-1543905105
@huangzhir thank you for the work. It seems the performance is poor for `FETCH_PRIOR`, for each request, we will reopen the log file and seek to taget position. Is there a real world use case for it ? If so, we might need to rewrite the code using `RandomAccessFile`. `FETCH_FIRST` looks more useful to me.
--
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] huangzhir commented on pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "huangzhir (via GitHub)" <gi...@apache.org>.
huangzhir commented on PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#issuecomment-1543578848
Hi @pan3793 @turboFei , I would appreciate it if you could take the time to review my code.
--
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] huangzhir commented on a diff in pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "huangzhir (via GitHub)" <gi...@apache.org>.
huangzhir commented on code in PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#discussion_r1217419207
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/operation/log/OperationLog.scala:
##########
@@ -172,6 +196,21 @@ class OperationLog(path: Path) {
toRowSet(logs)
}
+ private def resetReader(): Unit = {
+ lock.synchronized {
+ trySafely {
+ if (reader != null) {
+ reader.close()
+ }
+ }
+ reader = null
+ closeExtraReaders()
+ extraReaders.clear()
+ extraPaths.foreach(path =>
+ extraReaders += Files.newBufferedReader(path, StandardCharsets.UTF_8))
+ }
+ }
+
def read(from: Int, size: Int): TRowSet = synchronized {
Review Comment:
I had some additional considerations. Here, there is high concurrency and the probability of issues occurring is very low. I agree with your point that introducing a new object as a lock would add overhead and potentially lead to deadlock issues. I will remove the object lock.
--
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] huangzhir commented on a diff in pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "huangzhir (via GitHub)" <gi...@apache.org>.
huangzhir commented on code in PR #4813:
URL: https://github.com/apache/kyuubi/pull/4813#discussion_r1218124350
##########
kyuubi-common/src/test/scala/org/apache/kyuubi/log/SeekableBufferedReaderSuite.scala:
##########
@@ -94,6 +95,80 @@ class SeekableBufferedReaderSuite extends KyuubiFunSuite {
reader.close()
}
+ // borrow the test cases from FetchIteratorSuite
+ private def iteratorTest(fetchIter: FetchIterator[String]): Unit = {
+ fetchIter.fetchNext()
+ assert(fetchIter.getFetchStart == 0)
+ assert(fetchIter.getPosition == 0)
+ assertResult(0 until 2)(getRows(fetchIter, 2))
+ assert(fetchIter.getFetchStart == 0)
+ assert(fetchIter.getPosition == 2)
+
+ fetchIter.fetchNext()
+ assert(fetchIter.getFetchStart == 2)
+ assert(fetchIter.getPosition == 2)
+ assertResult(2 until 3)(getRows(fetchIter, 1))
+ assert(fetchIter.getFetchStart == 2)
+ assert(fetchIter.getPosition == 3)
+
+ fetchIter.fetchPrior(2)
Review Comment:
Appreciate the correction. I have removed the unrelated code.
--
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] ulysses-you closed pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you closed pull request #4813: Add the fetchorientation parameter to the /v1/operations/:operationId/log interface
URL: https://github.com/apache/kyuubi/pull/4813
--
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