keith-turner commented on PR #5490: URL: https://github.com/apache/accumulo/pull/5490#issuecomment-2822577082
> Overall, I think these changes are a good idea, however, this refactoring can very easily have bugs that are hard to spot. I'm not sure if this is the best for a 2.1 change, risking potentially introducing more bugs than the original PR set out to fix. @kevinrr888 I also like these changes and was a bit nervous about making the changes in 2.1. Personally I would like to try to get these changes in (because they make things more consistent which will reduce bugs long term, also the current inconsistency feels really buggy) and review our test coverage around this code. While reviewing this today I found a place I would like to add a unit test for RFileScanner. More generally we may want to eventually have something like #5474 that covers the iteratorEnv in all the different scanner types (like RFileScanner,OfflineScanner,ScannerImpl,BatchScanner,ClientSideIteratorScanner). We could try to as much as possible to run the same tests across all the different scanner types. For this PR I will try to run all the unit test w/ code coverage and see what the coverage looks like for the new code. -- 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: notifications-unsubscr...@accumulo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
