cestella commented on issue #1470: METRON-2193 Upgrade Enrichments for HBase 2.0.2 URL: https://github.com/apache/metron/pull/1470#issuecomment-521033037 This PR piqued my interest. :) First off, I'm glad to see we're fixing the use of the deprecated `HTables` and HBase APIs, so that's fantastic. Thanks for the effort on this @nickwallen . I will say, however, that I was surprised at the size of a single PR until I looked. This PR seems to be a mix of: * Dependency changes for HBase 2.0.2 * Rearchitecture/code abstraction rewriting (e.g. your point 11) * Replacing the deprecated API calls * Deprecating features I have some concerns about this approach. Conflating so many things inside of a PR which is already inherently risky seems to increase the risk multiplicatively. It is also difficult to review, I'd say. I would suggest that these three separate concerns be split across 3 separate PRs: * Replacing deprecated API calls against `master` * This is also a decent time to, instead of replacing the mistake of passing around `HTableInterface` to create a key-value store abstraction which you can pass around instead that supports scan, get and put. * Dependency changes for HBase 2.0.2 against this feature branch * Deprecating features * As I said in a previous comment, only after a community discussion and I'd strongly suggest it be separate from the upgrade. * Rearchitecture/code abstraction rewriting against `master` *after* the HDP upgrade has landed. As I say, please don't take my feedback as an indication that I don't appreciate the work that went into this. There is much good here, but there is just..well..very much here. :)
---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
