You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "bowenliang123 (via GitHub)" <gi...@apache.org> on 2023/08/14 14:21:20 UTC
[GitHub] [kyuubi] bowenliang123 opened a new pull request, #5161: [KYUUBI #3885] [Followup]Fix memory leak when using incremental collect mode in JDBC engine
bowenliang123 opened a new pull request, #5161:
URL: https://github.com/apache/kyuubi/pull/5161
<!--
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.
-->
- similar situation and reason in #3885
- `toIterable` creates a stream by `Stream.cons(self.next(), self.toStream)` which holds iterated rows of resultset in JDBC engine
### _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](https://kyuubi.readthedocs.io/en/master/contributing/code/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] bowenliang123 commented on pull request #5161: [WIP][KYUUBI #3885][Followup]Fix memory leak when using incremental collect mode in JDBC engine
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #5161:
URL: https://github.com/apache/kyuubi/pull/5161#issuecomment-1679844908
I may need a hand in this, for early closed result set and unset operationHandle. cc @zhaomin1423 @wForget
```
- test fetch orientation with incremental collect mode *** FAILED ***
org.apache.thrift.protocol.TProtocolException: Required field 'operationHandle' is unset! Struct:TGetOperationStatusReq(operationHandle:null)
at org.apache.hive.service.rpc.thrift.TGetOperationStatusReq.validate(TGetOperationStatusReq.java:373)
at org.apache.hive.service.rpc.thrift.TCLIService$GetOperationStatus_args.validate(TCLIService.java:12851)
at org.apache.hive.service.rpc.thrift.TCLIService$GetOperationStatus_args$GetOperationStatus_argsStandardScheme.write(TCLIService.java:12908)
at org.apache.hive.service.rpc.thrift.TCLIService$GetOperationStatus_args$GetOperationStatus_argsStandardScheme.write(TCLIService.java:12877)
at org.apache.hive.service.rpc.thrift.TCLIService$GetOperationStatus_args.write(TCLIService.java:12828)
at org.apache.thrift.TServiceClient.sendBase(TServiceClient.java:71)
at org.apache.thrift.TServiceClient.sendBase(TServiceClient.java:62)
at org.apache.hive.service.rpc.thrift.TCLIService$Client.send_GetOperationStatus(TCLIService.java:469)
at org.apache.hive.service.rpc.thrift.TCLIService$Client.GetOperationStatus(TCLIService.java:461)
at org.apache.kyuubi.operation.HiveJDBCTestHelper.waitForOperationToComplete(HiveJDBCTestHelper.scala:128)
...
OperationWithPhoenixEngineSuite:
- phoenix - get tables *** FAILED ***
org.apache.kyuubi.jdbc.hive.KyuubiSQLException: ResultSet closed
at org.apache.kyuubi.jdbc.hive.Utils.verifySuccess(Utils.java:90)
at org.apache.kyuubi.jdbc.hive.Utils.verifySuccessWithInfo(Utils.java:76)
at org.apache.kyuubi.jdbc.hive.KyuubiQueryResultSet.next(KyuubiQueryResultSet.java:330)
at org.apache.kyuubi.engine.jdbc.phoenix.PhoenixOperationSuite.$anonfun$new$2(PhoenixOperationSuite.scala:35)
at org.apache.kyuubi.engine.jdbc.phoenix.PhoenixOperationSuite.$anonfun$new$2$adapted(PhoenixOperationSuite.scala:30)
at org.apache.kyuubi.operation.JDBCTestHelper.$anonfun$withMultipleConnectionJdbcStatement$4(JDBCTestHelper.scala:61)
at org.apache.kyuubi.operation.JDBCTestHelper.$anonfun$withMultipleConnectionJdbcStatement$4$adapted(JDBCTestHelper.scala:61)
at scala.collection.mutable.ResizableArray.foreach(ResizableArray.scala:62)
at scala.collection.mutable.ResizableArray.foreach$(ResizableArray.scala:55)
at scala.collection.mutable.ArrayBuffer.foreach(ArrayBuffer.scala:49)
...
```
--
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] wForget commented on a diff in pull request #5161: [KYUUBI #3885] [Followup]Fix memory leak when using incremental collect mode in JDBC engine
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5161:
URL: https://github.com/apache/kyuubi/pull/5161#discussion_r1294108383
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala:
##########
@@ -67,7 +67,9 @@ class ExecuteStatement(
iter =
if (incrementalCollect) {
info("Execute in incremental collect mode")
- new IterableFetchIterator(resultSetWrapper.toIterable)
+ new IterableFetchIterator[Row](new Iterable[Row] {
Review Comment:
This will cause the resultSet to be fetched lazily, so we can not close the resultSet in the finally block.
https://github.com/apache/kyuubi/blob/938384c2259b361cdf1b3bba81f615064289817c/externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala#L91-L94
--
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] wForget commented on a diff in pull request #5161: [KYUUBI #3885] [Followup]Fix memory leak when using incremental collect mode in JDBC engine
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5161:
URL: https://github.com/apache/kyuubi/pull/5161#discussion_r1294108383
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala:
##########
@@ -67,7 +67,9 @@ class ExecuteStatement(
iter =
if (incrementalCollect) {
info("Execute in incremental collect mode")
- new IterableFetchIterator(resultSetWrapper.toIterable)
+ new IterableFetchIterator[Row](new Iterable[Row] {
Review Comment:
This will cause the resultSet to be fetched lazily, so we can not close the statement in the finally block.
https://github.com/apache/kyuubi/blob/938384c2259b361cdf1b3bba81f615064289817c/externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala#L91-L94
--
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] wForget commented on pull request #5161: [WIP][KYUUBI #3885][Followup]Fix memory leak when using incremental collect mode in JDBC engine
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on PR #5161:
URL: https://github.com/apache/kyuubi/pull/5161#issuecomment-1680784711
sql syntax error:
```
09:49:40.433 JdbcTBinaryFrontendHandler-Pool: Thread-87 INFO ExecuteStatement: Processing runner's query[4bda9ddb-e5f3-4a75-ae3f-6726949eedfe]: PENDING_STATE -> RUNNING_STATE, statement:
select e1 from (select 1 k1) as t lateral view explode([0,1]) tmp1 as e1
09:49:40.458 JdbcTBinaryFrontendHandler-Pool: Thread-87 ERROR ExecuteStatement: Error operating ExecuteStatement: java.sql.SQLException: errCode = 2, detailMessage = Syntax error in line 1:
...as t lateral view explode([0,1]) tmp1 as e1
^
Encountered: [
Expected: IDENTIFIER
at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:129)
at com.mysql.cj.jdbc.exceptions.SQLExceptionsMapping.translateException(SQLExceptionsMapping.java:122)
at com.mysql.cj.jdbc.StatementImpl.executeInternal(StatementImpl.java:763)
at com.mysql.cj.jdbc.StatementImpl.execute(StatementImpl.java:648)
at org.apache.kyuubi.engine.jdbc.operation.ExecuteStatement.org$apache$kyuubi$engine$jdbc$operation$ExecuteStatement$$executeStatement(ExecuteStatement.scala:64)
at org.apache.kyuubi.engine.jdbc.operation.ExecuteStatement.runInternal(ExecuteStatement.scala:55)
at org.apache.kyuubi.operation.AbstractOperation.run(AbstractOperation.scala:171)
at org.apache.kyuubi.session.AbstractSession.runOperation(AbstractSession.scala:101)
at org.apache.kyuubi.session.AbstractSession.$anonfun$executeStatement$1(AbstractSession.scala:131)
at org.apache.kyuubi.session.AbstractSession.withAcquireRelease(AbstractSession.scala:82)
at org.apache.kyuubi.session.AbstractSession.executeStatement(AbstractSession.scala:128)
at org.apache.kyuubi.service.AbstractBackendService.executeStatement(AbstractBackendService.scala:67)
at org.apache.kyuubi.service.TFrontendService.ExecuteStatement(TFrontendService.scala:252)
at org.apache.hive.service.rpc.thrift.TCLIService$Processor$ExecuteStatement.getResult(TCLIService.java:1557)
at org.apache.hive.service.rpc.thrift.TCLIService$Processor$ExecuteStatement.getResult(TCLIService.java:1542)
at org.apache.thrift.ProcessFunction.process(ProcessFunction.java:39)
at org.apache.thrift.TBaseProcessor.process(TBaseProcessor.java:39)
at org.apache.kyuubi.service.authentication.TSetIpAddressProcessor.process(TSetIpAddressProcessor.scala:36)
at org.apache.thrift.server.TThreadPoolServer$WorkerProcess.run(TThreadPoolServer.java:286)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:750)
java.sql.SQLException: errCode = 2, detailMessage = Syntax error in line 1:
...as t lateral view explode([0,1]) tmp1 as e1
```
--
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] wForget commented on a diff in pull request #5161: [KYUUBI #3885][FOLLOWUP] Fix memory leak when using incremental collect mode in JDBC engine
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5161:
URL: https://github.com/apache/kyuubi/pull/5161#discussion_r1302350288
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala:
##########
@@ -67,9 +70,23 @@ class ExecuteStatement(
iter =
if (incrementalCollect) {
info("Execute in incremental collect mode")
- new IterableFetchIterator(resultSetWrapper.toIterable)
+ new IterableFetchIterator[Row](new Iterable[Row] {
+ private val fetched = new AtomicBoolean(false)
+ override def iterator: Iterator[Row] = {
+ if (fetched.getAndSet(true)) {
+ jdbcStatement.synchronized {
Review Comment:
For update statement `hasResult` should be `false`.
https://github.com/apache/kyuubi/blob/74416e935e565a760b09ddc259365d73e05aeb85/externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala#L66
--
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] wForget commented on a diff in pull request #5161: [WIP][KYUUBI #3885][Followup]Fix memory leak when using incremental collect mode in JDBC engine
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5161:
URL: https://github.com/apache/kyuubi/pull/5161#discussion_r1296992037
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/util/ResultSetWrapper.scala:
##########
@@ -30,6 +30,7 @@ class ResultSetWrapper(statement: Statement)
private lazy val metadata = currentResult.getMetaData
override def hasNext: Boolean = {
+ if (currentResult.isClosed) return false
Review Comment:
`FetchResult` may be called again when the previous resultSet has ended, so we need to determine whether the resultSet has been closed.
https://github.com/apache/kyuubi/blob/f05aefb86643e69c64237ebfb9db208ec509f3b2/kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/KyuubiQueryResultSet.java#L328
--
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] bowenliang123 commented on pull request #5161: [WIP][KYUUBI #3885][Followup]Fix memory leak when using incremental collect mode in JDBC engine
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #5161:
URL: https://github.com/apache/kyuubi/pull/5161#issuecomment-1679847149
I may need a hand in this, for early closed result set and unset operationHandle. cc @zhaomin1423 @wForget
ci log: https://github.com/apache/kyuubi/actions/runs/5863404047/job/15896805347?pr=5161#step:7:272
```
- test fetch orientation with incremental collect mode *** FAILED ***
org.apache.thrift.protocol.TProtocolException: Required field 'operationHandle' is unset! Struct:TGetOperationStatusReq(operationHandle:null)
at org.apache.hive.service.rpc.thrift.TGetOperationStatusReq.validate(TGetOperationStatusReq.java:373)
at org.apache.hive.service.rpc.thrift.TCLIService$GetOperationStatus_args.validate(TCLIService.java:12851)
at org.apache.hive.service.rpc.thrift.TCLIService$GetOperationStatus_args$GetOperationStatus_argsStandardScheme.write(TCLIService.java:12908)
at org.apache.hive.service.rpc.thrift.TCLIService$GetOperationStatus_args$GetOperationStatus_argsStandardScheme.write(TCLIService.java:12877)
at org.apache.hive.service.rpc.thrift.TCLIService$GetOperationStatus_args.write(TCLIService.java:12828)
at org.apache.thrift.TServiceClient.sendBase(TServiceClient.java:71)
at org.apache.thrift.TServiceClient.sendBase(TServiceClient.java:62)
at org.apache.hive.service.rpc.thrift.TCLIService$Client.send_GetOperationStatus(TCLIService.java:469)
at org.apache.hive.service.rpc.thrift.TCLIService$Client.GetOperationStatus(TCLIService.java:461)
at org.apache.kyuubi.operation.HiveJDBCTestHelper.waitForOperationToComplete(HiveJDBCTestHelper.scala:128)
...
OperationWithPhoenixEngineSuite:
- phoenix - get tables *** FAILED ***
org.apache.kyuubi.jdbc.hive.KyuubiSQLException: ResultSet closed
at org.apache.kyuubi.jdbc.hive.Utils.verifySuccess(Utils.java:90)
at org.apache.kyuubi.jdbc.hive.Utils.verifySuccessWithInfo(Utils.java:76)
at org.apache.kyuubi.jdbc.hive.KyuubiQueryResultSet.next(KyuubiQueryResultSet.java:330)
at org.apache.kyuubi.engine.jdbc.phoenix.PhoenixOperationSuite.$anonfun$new$2(PhoenixOperationSuite.scala:35)
at org.apache.kyuubi.engine.jdbc.phoenix.PhoenixOperationSuite.$anonfun$new$2$adapted(PhoenixOperationSuite.scala:30)
at org.apache.kyuubi.operation.JDBCTestHelper.$anonfun$withMultipleConnectionJdbcStatement$4(JDBCTestHelper.scala:61)
at org.apache.kyuubi.operation.JDBCTestHelper.$anonfun$withMultipleConnectionJdbcStatement$4$adapted(JDBCTestHelper.scala:61)
at scala.collection.mutable.ResizableArray.foreach(ResizableArray.scala:62)
at scala.collection.mutable.ResizableArray.foreach$(ResizableArray.scala:55)
at scala.collection.mutable.ArrayBuffer.foreach(ArrayBuffer.scala:49)
...
```
--
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] bowenliang123 closed pull request #5161: [KYUUBI #3885] [Followup]Fix memory leak when using incremental collect mode in JDBC engine
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 closed pull request #5161: [KYUUBI #3885] [Followup]Fix memory leak when using incremental collect mode in JDBC engine
URL: https://github.com/apache/kyuubi/pull/5161
--
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] wForget commented on a diff in pull request #5161: [WIP][KYUUBI #3885][Followup]Fix memory leak when using incremental collect mode in JDBC engine
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5161:
URL: https://github.com/apache/kyuubi/pull/5161#discussion_r1296943061
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala:
##########
@@ -67,9 +70,23 @@ class ExecuteStatement(
iter =
if (incrementalCollect) {
info("Execute in incremental collect mode")
- new IterableFetchIterator(resultSetWrapper.toIterable)
+ new IterableFetchIterator[Row](new Iterable[Row] {
+ private val fetched = new AtomicBoolean(false)
+ override def iterator: Iterator[Row] = {
+ if (fetched.getAndSet(true)) {
+ jdbcStatement.synchronized {
Review Comment:
we need to re-execute the statement when resetPosition
--
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] cxzl25 commented on a diff in pull request #5161: [KYUUBI #3885][FOLLOWUP] Fix memory leak when using incremental collect mode in JDBC engine
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on code in PR #5161:
URL: https://github.com/apache/kyuubi/pull/5161#discussion_r1301187412
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala:
##########
@@ -67,9 +70,23 @@ class ExecuteStatement(
iter =
if (incrementalCollect) {
info("Execute in incremental collect mode")
- new IterableFetchIterator(resultSetWrapper.toIterable)
+ new IterableFetchIterator[Row](new Iterable[Row] {
+ private val fetched = new AtomicBoolean(false)
+ override def iterator: Iterator[Row] = {
+ if (fetched.getAndSet(true)) {
+ jdbcStatement.synchronized {
Review Comment:
Would re-execution be a possible problem? For example, insert is executed twice.
--
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
Re: [PR] [KYUUBI #3885][FOLLOWUP] Fix memory leak when using incremental collect mode in JDBC engine [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #5161:
URL: https://github.com/apache/kyuubi/pull/5161#issuecomment-1788994514
Closing this PR, as https://github.com/apache/kyuubi/pull/5570 is merged for fixing the same 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] bowenliang123 commented on a diff in pull request #5161: [KYUUBI #3885] [Followup]Fix memory leak when using incremental collect mode in JDBC engine
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5161:
URL: https://github.com/apache/kyuubi/pull/5161#discussion_r1294116266
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala:
##########
@@ -67,7 +67,9 @@ class ExecuteStatement(
iter =
if (incrementalCollect) {
info("Execute in incremental collect mode")
- new IterableFetchIterator(resultSetWrapper.toIterable)
+ new IterableFetchIterator[Row](new Iterable[Row] {
Review Comment:
Thanks for the hint. It might be the cause for the failure of closing statement too early. (https://github.com/apache/kyuubi/actions/runs/5857069896/job/15886592243#step:7:292)
Let me reopen this PR to have another try.
--
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 #5161: [WIP][KYUUBI #3885][Followup]Fix memory leak when using incremental collect mode in JDBC engine
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5161:
URL: https://github.com/apache/kyuubi/pull/5161#issuecomment-1678437839
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/5161?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#5161](https://app.codecov.io/gh/apache/kyuubi/pull/5161?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (5b4fb74) into [master](https://app.codecov.io/gh/apache/kyuubi/commit/9dcb61e00dedfd5fea166c7f01b4727c6082f85d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (9dcb61e) will **not change** coverage.
> Report is 1 commits behind head on master.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #5161 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 566 566
Lines 31624 31624
Branches 4122 4122
======================================
Misses 31624 31624
```
: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
Re: [PR] [KYUUBI #3885][FOLLOWUP] Fix memory leak when using incremental collect mode in JDBC engine [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 closed pull request #5161: [KYUUBI #3885][FOLLOWUP] Fix memory leak when using incremental collect mode in JDBC engine
URL: https://github.com/apache/kyuubi/pull/5161
--
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] wForget commented on a diff in pull request #5161: [KYUUBI #3885][Followup]Fix memory leak when using incremental collect mode in JDBC engine
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5161:
URL: https://github.com/apache/kyuubi/pull/5161#discussion_r1296943061
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala:
##########
@@ -67,9 +70,23 @@ class ExecuteStatement(
iter =
if (incrementalCollect) {
info("Execute in incremental collect mode")
- new IterableFetchIterator(resultSetWrapper.toIterable)
+ new IterableFetchIterator[Row](new Iterable[Row] {
+ private val fetched = new AtomicBoolean(false)
+ override def iterator: Iterator[Row] = {
+ if (fetched.getAndSet(true)) {
+ jdbcStatement.synchronized {
Review Comment:
we need to re-execute the statement when resetPosition is called
--
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] wForget commented on a diff in pull request #5161: [KYUUBI #3885] [Followup]Fix memory leak when using incremental collect mode in JDBC engine
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5161:
URL: https://github.com/apache/kyuubi/pull/5161#discussion_r1294108383
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala:
##########
@@ -67,7 +67,9 @@ class ExecuteStatement(
iter =
if (incrementalCollect) {
info("Execute in incremental collect mode")
- new IterableFetchIterator(resultSetWrapper.toIterable)
+ new IterableFetchIterator[Row](new Iterable[Row] {
Review Comment:
This will cause the resultSet to be fetched lazily, so we can not close the statementin the finally block.
https://github.com/apache/kyuubi/blob/938384c2259b361cdf1b3bba81f615064289817c/externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala#L91-L94
--
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