capistrant edited a comment on pull request #11429: URL: https://github.com/apache/druid/pull/11429#issuecomment-934718216
> > > seems reasonable to me. > > > What sort of integration test do you think would be needed to validate this change? > > > > > > Ya, I have continued to think about an integration test here but I am not sure there is anything straightforward. In a live cluster, the only way to test that the context override is honored would be by analyzing query metric output or query log output (at least that I can think of). So I plan to instead look at DruidSchema for opportunities to test that the config is honored > > Sounds good to me. I think if there is a unit test that validates the context in BrokerInternalQueryConfigTest is used by the DruidSchema#runSegmentMetadataQuery method - that should be sufficient testing to get this change merged. I ended up refactoring a bit. After looking at the code, I didn't really get why DruidSchema#runSegmentMetadataQuery was a static method so I changed it to an instance method and ended up exposing that for test. I could certainly undo that and think I could still test successfully. I added a test that confirms the query is submitted with proper context override. The test is a little big ugly since I couldn't leverage the existing schema object used in junit setup. I needed a DruidSchema object with the lifecycle factory and the underlying lifecycle as mock objects and the existing schema did not provide. -- 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]
