[GitHub] [phoenix-omid] gjacoby126 commented on a diff in pull request #109: OMID-223 Refactor Omid to use HBase 2 APIs internally.

[GitBox <gi...@apache.org>]

gjacoby126 commented on code in PR #109:
URL: https://github.com/apache/phoenix-omid/pull/109#discussion_r947220162


##########
hbase-coprocessor/src/main/java/org/apache/omid/transaction/CellSkipFilterBase.java:
##########
@@ -53,21 +53,21 @@ public CellSkipFilterBase(Filter filter) {
      */
     private boolean skipCellVersion(Cell cell) {
         return skipColumn != null
-        && CellUtil.matchingRow(cell, skipColumn.getRowArray(), skipColumn.getRowOffset(),
+        && PrivateCellUtil.matchingRows(cell, skipColumn.getRowArray(), skipColumn.getRowOffset(),

Review Comment:
   @stoty @Aarchy - Agreed. I also don't see an alternative to using an IA.Private class here. Given the choice between KeyValueUtil / KeyValue, and the PrivateCellUtil's createFirstOnRow function, I think the latter's the better option. 
   
   It keeps the interface encapsulation around Cell and doesn't require Phoenix code to make assumptions about whether the Cell is an array-backed on-heap object or an off-heap byte-buffer backed object. 
   
   Rushabh Shah and I once asked to allow PrivateCellUtil to be IA.LimitedPrivate(COPROC) on HBASE-25328, which the HBase community was not willing to do.  Instead they were willing to expose new LimitedPrivate helper methods in HBase that called particular IA.Private methods we needed. Probably not a good time with HBase 2.5 about to release, but that's something we could pursue in the future. 
   
   This does of course mean we're accepting the risk that this method can change arbitrarily with every HBase upgrade.  



-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org