PHOENIX-6776 :- Abort scans of closed connections at ScanningResultIterator #1517
Conversation
phoenix-core/src/it/java/org/apache/phoenix/end2end/PreMatureTimelyAbortScanIt.java
Show resolved
Hide resolved
| @Override | ||
| public void run() { | ||
| try { | ||
| ResultSet resultSet = conn.createStatement().executeQuery( |
There was a problem hiding this comment.
Doesn't this thread need to loop or wait on the other thread to close the connection first? What if Thread t gets scheduled to run to completion (hence failure) before t1 does anything?
There was a problem hiding this comment.
I checked the time for completing query was more that 1500ms, but true that was on local, will add wait for proper conn closing thanks!
There was a problem hiding this comment.
Another option to consider is whether you can do a synchronous test by, for example, mocking a Connection to always return isClosing = true, isClosed = false, and verifying that ScanningResultIterator does what you expect (runs its close logic)
Fixing the asynchronous logic to be deterministic through locking as you suggest also works.
There was a problem hiding this comment.
@lokiore - The Thread.sleep calls still make this non-deterministic, don't they?
There was a problem hiding this comment.
@lokiore, in general we do not want to use thread sleep as we can only guess the required sleep time and it changes from one system to another. This can make the test flapping. You can implement this test without having threads. In PhoenixConnection, you can define a testing only method to set the connection state to closing. After getting the connection, set the connection state closing using this method. Then you run the test code without creating threads and without closing the connection.
There was a problem hiding this comment.
Thanks @gjacoby126 @kadirozde for suggesting, I have made changes Please take a look, Thanks
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/BaseScannerRegionObserver.java
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java
Show resolved
Hide resolved
| @@ -190,4 +209,9 @@ public String toString() { | |||
| public ResultScanner getScanner() { | |||
| return scanner; | |||
| } | |||
|
|
|||
| @VisibleForTesting | |||
| public static void setIsScannerClosedForceFully(boolean throwException) { | |||
There was a problem hiding this comment.
tiny nit: "forcefully" is one word so only the first F should be capitalized
lokiore
requested review from
gjacoby126 and
kadirozde
and removed request for
gjacoby126 and
kadirozde
|
I am trying to re-request review from both @gjacoby126 and @kadirozde but somehow it is removing one when I add other 😓, please take a look, Thanks!! |
| int amount = -50000 + i; | ||
| String s = "UPSERT INTO LONG_BUG (ID, AMOUNT) VALUES( " + i + ", " + amount + ")"; | ||
| conn.createStatement().execute(s); | ||
| conn.commit(); |
There was a problem hiding this comment.
It will be faster if you move this commit out of the loop
There was a problem hiding this comment.
Also we should not need to add 5000 rows for this test after the recent changes. Let's reduce it to say 100.
There was a problem hiding this comment.
Pushed the change Thanks for pointing.
| @@ -1374,6 +1381,11 @@ public void setTableResultIteratorFactory(TableResultIteratorFactory factory) { | |||
| this.tableResultIteratorFactory = factory; | |||
| } | |||
|
|
|||
| @VisibleForTesting | |||
There was a problem hiding this comment.
Can you add a comment for this method is that it is only used for testing?
…erator (apache#1517) Co-authored-by: Lokesh Khurana <lokesh.khurana@salesforce.com>
|
@lokiore This change is causing regression in IndexScrutiny tool which in turn is causing tests to fail |
Added isClosing property in PhoenixConnection as well, as we change isClose status after closing all the underline objects (Iterators, scanners, statements).