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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
