nickwallen commented on issue #1470: METRON-2193 Upgrade Enrichments for HBase 2.0.2 URL: https://github.com/apache/metron/pull/1470#issuecomment-521041113 > I will say, however, that I was surprised at the size of a single PR until I looked... 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 do agree with you @cestella that this PR is large and I very much prefer small, isolated PRs. The HDP-3.1 upgrade has been a huge effort so far and I've tried to break it down into small, reviewable PRs. Here are all the PRs that we've been able to land so far as part of the upgrade. * #1395 * #1397 * #1447 * #1448 * #1451 * #1454 * #1456 * #1457 * #1458 I tried to follow that similar pattern prior to opening this PR and ran into some problems. I attempted to open separate PRs for each functional area included here. Something along the lines of... * Enrichment Coprocessor * Legacy Adapters and Stellar Enrichment functions * Data management; TAXII, CSV loaders But since (1) the changes in the areas listed below are not backwards compatible (unlike all the other preceding PRs listed above) and (2) there are many interdependencies between these areas, I was not able to submit separate PRs that would actually compile. To help cut the fat and reduce the size of this PR, there are some changes here that I could try to undo or extract into separate PRs. These come to mind immediately 1. We have a public field accessed in `EnrichmentKey`. This was changed to getters and it ended up impacting a lot of files. 1. `LookupKV<KEY_T, VALUE_T>` -> `EnrichmentResult`. I actually tried to 'undo' this before submitting this PR and I ran into a problem. I can try to to tackle this again so I can at least describe why this might be needed, if not address it. 1. The deprecation of "Least Recently Used Pruner" (assuming that is acceptable to the community post-discuss) would have to come before this PR. > Rearchitecture/code abstraction rewriting against.. Do the items I listed above (1) public field access and (2) LookupKV -> EnrichmentResult cover what you mean by rearchitecture? Are there other items that you are thinking of that fall under this heading?
---------------------------------------------------------------- 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
