[ 
https://issues.apache.org/jira/browse/HBASE-9754?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13798086#comment-13798086
 ] 

stack commented on HBASE-9754:
------------------------------

What are the implications of this change?

-    MultiVersionConsistencyControl.resetThreadReadPoint();
     Scan scan = new Scan(get);
     InternalScanner scanner = (InternalScanner) store.getScanner(scan,
-        scan.getFamilyMap().get(store.getFamily().getName()));
+        scan.getFamilyMap().get(store.getFamily().getName()), 0);

We no longer do reset and we pass 0 which means all is readable?  Would think 
we'd retain at least the first line?

Does defaulting 0 in all tests change what is being tested (or do you think the 
reset was setting readpoint to 0 across the board anyways)?

This should be final now?

+  private long readPt = 0;

Is this an odd 'public' method to have in HRegion?

+   /*
+    * Returns readpoint considering given IsolationLevel
+    */
+   public long getReadpoint(IsolationLevel isolationLevel) {
+     if (isolationLevel == IsolationLevel.READ_UNCOMMITTED) {
+       // This scan can read even uncommitted transactions
+       return Long.MAX_VALUE;
+     }
+     return mvcc.memstoreReadPoint();
+   }

especially as given just above, we can do a getMVCC so can go 
'behind-the-scenes'.  Should it be private?

It is the readPoint for the region?

Remove

-        MultiVersionConsistencyControl.setThreadReadPoint(this.readPt);
+        // MultiVersionConsistencyControl.setThreadReadPoint(this.readPt);

ditto

-        MultiVersionConsistencyControl.setThreadReadPoint(this.readPt);
+        // MultiVersionConsistencyControl.setThreadReadPoint(this.readPt);

Undocumented passing of zero (explaining why this is ok) and the sometimes 
passing of smallestreadpoint instead of readpoint is unsettling.  Explaination 
on why it is ok in the former case and what smallest means in latter would help 
the folks who come after understand what is going on.

Otherwise patch is good.



> Consider eliminating threadlocals from MVCC code
> ------------------------------------------------
>
>                 Key: HBASE-9754
>                 URL: https://issues.apache.org/jira/browse/HBASE-9754
>             Project: HBase
>          Issue Type: Bug
>            Reporter: Lars Hofhansl
>            Assignee: Ted Yu
>             Fix For: 0.98.0
>
>         Attachments: 9754-rp-0.txt, 9754-rp-1.txt, 9754-rp-hregion.txt, 
> 9754-rp-hregion-v2.txt, 9754-rp-hregion-v3.txt
>
>
> Brought up by [~vrodionov] and [~yuzhih...@gmail.com].
> Currently we use ThreadLocals to communicate the current readpoint between a 
> RegionScanner and the Store\{File}Scanner's down the stack.
> Since ThreadLocals are not cheap we should consider whether it is possible to 
> pass the readpoint through the call stack instead.



--
This message was sent by Atlassian JIRA
(v6.1#6144)

Reply via email to