phoenix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From joshelser <...@git.apache.org>
Subject [GitHub] phoenix pull request: PHOENIX-2743 Hive Storage support
Date Mon, 11 Apr 2016 20:22:04 GMT
Github user joshelser commented on the pull request:

    https://github.com/apache/phoenix/pull/155#issuecomment-208542873
  
    Some general thoughts (I stopped leaving them inline everytime I saw them). I'm guessing
you "inherited" some of these from JeongMin's original work.
    
    * Dbl-check indentations
    * Try to remove commented out code
    * Some class-level javadoc comments would be *amazing*
    * Not a single unit test? :)
    
    Other things that I remember biting me previously:
    
    * Make sure you try to run with Tez as well. Both in the "uber" (local job) mode and a
normal tez task. There are.. subtleties between them, sadly (as sadly, I don't remember the
specifics anymore).
    
    Other general thoughts:
    * The RecordUpdater implementation looks pretty cool. Didn't know they made this available
for StorageHandlers.
    * Hive has a decent suite for running Hive tests as a part of their build (which includes
tests for StorageHandlers) with this qtest/itest modules. You might be able to take some inspiration
from these for testing.
    
    Looks good so far. It will be a nice bridge between Phoenix and Hive (as we work towards
a common-core of Calcite).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message