kevinrr888 commented on PR #4816: URL: https://github.com/apache/accumulo/pull/4816#issuecomment-2815778297
@dlmarion - This was preexisting functionality added in https://github.com/apache/accumulo/pull/955/, and may be explained there. I think I went through this PR a while ago, but don't remember anything about it. Skimming through some of the old comments in this PR, it looks like I address some of the difficulties with this current impl, as you mentioned. > Comment about IteratorEnvironment.getConfig(): TLDR: opted for not having a default impl for IteratorEnvironment.getConfig() (keep default as throwing UOE) It might be strange to have a default impl for this method but not others... If we do want a default impl for this that isnt just throwing a UOE, we should probably change the others too... But that should be done in a follow on if that is desired. Keeping the default as throwing a UOE keeps it consistent with the other default impls/more inline with the original approach (throw UOE by default, concrete impls should have an actual impl). The only IteratorEnvironment which would be using the default getConfig() would be ClientSideIteratorScanner. But we can just add the impl there (done in latest commit). It is also hard to know what would be a good default impl since its not exactly clear how getConfig() was originally working. So seems like changing IteratorEnvironment completely in this PR might be a scope creep (at least that's I thought 7 months ago) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
