NihalJain commented on PR #15861: URL: https://github.com/apache/pinot/pull/15861#issuecomment-2930978998
> The refactor definitely reduces duplicate code by > > * Moving part of the common code to `BasicAuthUtils.java`. > > * Moving other parts to respective base classes. > > > > The major open question is testing. I am not sure if there is good enough automated coverage. Thank you for looking @vrajat I have been writing tests, havenot pushed them yet, will push the changes soon. occupied with some other work. I plan yo raise another PR just for some tests and rebase and ensure this patch doesnot break anything. Have found a few bugs in existing code as well, will push them as well. we can decide if should do as separate ticket. > One more (nit) comment, Abstract classes are usually prefixed with `Base`. So `AbstractBasicAuthAccessControl` should be called `BaseBasicAuthAccessControl`. Yes will handle this. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
