HBASE-27658: Add max live time limit for cached connections

[wForget]

Add max live time limit for cached connections to avoid token expiration for long active connections

[Apache-HBase]

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 4s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s	The patch appears to include 1 new or modified test files.
		_ master Compile Tests _
+1 💚	mvninstall	1m 41s	master passed
+1 💚	compile	0m 38s	master passed
+1 💚	scaladoc	0m 51s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	1m 7s	the patch passed
+1 💚	compile	0m 51s	the patch passed
+1 💚	scalac	0m 51s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	scaladoc	1m 1s	the patch passed
		_ Other Tests _
+1 💚	unit	8m 44s	hbase-spark in the patch passed.
		16m 57s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-118/1/artifact/yetus-precommit-check/output/Dockerfile
GITHUB PR	#118
Optional Tests	dupname scalac scaladoc unit compile
uname	Linux da538aad1ba0 5.4.0-1101-aws #109~18.04.1-Ubuntu SMP Mon Apr 24 20:40:49 UTC 2023 x86_64 GNU/Linux
Build tool	hb_maven
Personality	dev-support/jenkins/hbase-personality.sh
git revision	master / f8ee8f6
Test Results	https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-118/1/testReport/
Max. process+thread count	956 (vs. ulimit of 12500)
modules	C: spark/hbase-spark U: spark/hbase-spark
Console output	https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-118/1/console
versions	git=2.20.1
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

[Reidddddd]

so with this patch, even live connections will be cleaned up?

isn't the purpose to clean the token expired connections?

[wForget]

so with this patch, even live connections will be cleaned up?

isn't the purpose to clean the token expired connections?

The connections that are being referenced will not be cleaned up.

hbase-connectors/spark/hbase-spark/src/main/scala/org/apache/hadoop/hbase/spark/HBaseConnectionCache.scala

Lines 122 to 123 in 4415f28

	toCloseConnectionMap.foreach { x =>
	if (forceClean || x._2.refCount <= 0) {

After the connection creation time exceeds maxLifeTime, it will be moved into toCloseConnectionMap, which means it will no longer be newly referenced. The connection in toCloseConnectionMap is only released after there is no reference, and will not affect the active connection.

[wForget]

so with this patch, even live connections will be cleaned up?

isn't the purpose to clean the token expired connections?

This PR does not need to clean up the expired token connection, but to avoid token expiration by controlling the maximum usage time of the connection. The spark driver will periodically renew the token, if we do not reuse the connection for a long time, we can avoid the token expiration problem. It also seems difficult for us to judge whether the token of the connection has expired.

[Reidddddd]

so with this patch, even live connections will be cleaned up?
isn't the purpose to clean the token expired connections?

The connections that are being referenced will not be cleaned up.

hbase-connectors/spark/hbase-spark/src/main/scala/org/apache/hadoop/hbase/spark/HBaseConnectionCache.scala

Lines 122 to 123 in 4415f28

	toCloseConnectionMap.foreach { x =>
	if (forceClean || x._2.refCount <= 0) {

After the connection creation time exceeds maxLifeTime, it will be moved into toCloseConnectionMap, which means it will no longer be newly referenced. The connection in toCloseConnectionMap is only released after there is no reference, and will not affect the active connection.

the connection itself has a parameter to judge whether it is idle and to be cleaned up, can that para be used?

[wForget]

the connection itself has a parameter to judge whether it is idle and to be cleaned up, can that para be used?

For example, in the stream task, we execute a task to write to hbase every 5s, and setting the idle time of the connection to 10s means that the connection will not be cleaned up due to idleness.

[Reidddddd]

not really understand and still confused

what's the difference with this block

          if(forceClean || ((x._2.refCount <= 0) && (tsNow - x._2.timestamp > timeout))) {
            try{
              x._2.connection.close()
            } catch {
              case e: IOException => logWarning(s"Fail to close connection ${x._2}", e)
            }
            connectionMap.remove(x._1)
          }

it also has a timeout para, also remove from connectionMap.

why still need to introduce max live limit.

[wForget]

it also has a timeout para, also remove from connectionMap.

why still need to introduce max live limit.

As in the previous example, SmartConnection.timestamp is updated when the connection is closed. If I generate a task every 5s and SmartConnection.timestamp is updated after completion, then tsNow - x._2.timestamp > timeout will never be triggered.

[Reidddddd]

is updated when the connection is closed

hmm, sounds like this logic is wrong, can we just update the timestamp logic here instead of introducing a new para

[wForget]

sounds like this logic is wrong

I prefer that they are different logics.

can we just update the timestamp logic here instead of introducing a new para

Do you have any improvement suggestions?

[Reidddddd]

then tsNow - x._2.timestamp > timeout will never be triggered.

it is a wrong if your sayings is correct.

and in fact what it does it just same to yours, to close timeout connections, so you could just modify the existed one to a correct one

[wForget]

then tsNow - x._2.timestamp > timeout will never be triggered.

it is a wrong if your sayings is correct.

and in fact what it does it just same to yours, to close timeout connections, so you could just modify the existed one to a correct one

The timeout in the current logic means that the connection will be released after being idle for a period of time.

For a batch application that accesses hbase for a period of time and then performs other work, it can reuse the connection and release it when appropriate. But for a continuously active streaming application, we cannot release the connection through idle timeout but add a maximum lifetime limit.

I think both parameters are needed, they are suitable for different scenarios.