jihoonson commented on pull request #10920: URL: https://github.com/apache/druid/pull/10920#issuecomment-883624912
> > @jihoonson could you create a branch in the druid repo for me please? > > @JulianJaffePinterest Here you go - https://github.com/apache/druid/tree/spark_druid_connector Sorry @JulianJaffePinterest, I missed your comment. @abhishekagarwal87 thank you for creating a branch. > @jihoonson I don't have the ability to create a branch in this repo to open prs against, but I'm game to try. It's been difficult to get reviews from committers (it took two months and multiple emails to the dev list to get the first review for this diff) and so I'm hesitant to go down a path of needing to hunt reviews for many diffs, but if you think smaller prs will actually get reviews I can do so. (As an aside, if the Druid website's [list of committers](https://druid.apache.org/community/) is accurate only 1 of the last 10 prs tagged "Design Review" actually had approval from three committers in GitHub, so some process improvements may be necessary) I'm fine with either working on a branch or merging this pr directly. Whatever the fastest way for merging this change would be best to me. Since @samarthjain already reviewed this PR, you need 2 more approvals from committers. So, it's up to you which way you choose. I suggested working on a branch because committers are usually busy and big PRs usually hard to get reviews. For the design review process, I believe we are doing better than you described. I checked some of last PRs tagged design review, and they do have 3 approvals from committers, including the PR author himself. Perhaps the committer list on our website has gone stale. You can find the most recent list of committers in https://people.apache.org/phonebook.html?unix=druid. (We probably should add this link in our website instead of our own committer list) > For documenting level of confidence in the various deep storage/metadata options, would you prefer an explicit table in the docs or in-line callouts (the current approach). Oh, I didn't realize they are called out! Thank you for adding them. Perhaps I didn't read the doc carefully. But it might mean that it could be better if the callout was more obvious or more emphasized. I'm fine with either a table or callouts, but prefer whatever the most explicit way. > I haven't updated .travis.yml beyond the checkstyle because the existing travis packages run the static checks and tests for the new module. In addition to unit tests, there's a light end-to-end test as well that writes segments out and then reads the created segments and confirms they match what was written. If there's actually a road for this pr, I can look into writing an integration test that requires an external spark cluster but obviously that won't be run as part of travis. These tests sound really nice. I think it's OK to not have integration tests or not run those unit tests automatically in this PR, but we should do it as soon as possible. Do you have a follow-up plan for 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]
