jihoonson commented on pull request #10920: URL: https://github.com/apache/druid/pull/10920#issuecomment-881756176
> @jihoonson @samarthjain has been reviewing the pr but yes I agree it's too large to effectively review. I'm not sure how to reduce it to pieces though - splitting it into just readers or just writers still means that the first pr is very large (since it would still need all the shared code). Do you have any suggestions there? @JulianJaffePinterest I don't fully understand how this change is structured as I'm unfamiliar with scala, so it is hard for me to suggest something good based on code structure. An idea is creating a branch for this project and committing changes to there until most of changes are committed. Since you are going to create a PR against a branch, the PR doesn't have to be atomic, but can have incomplete changes or features. This can make each PR smaller. When the branch is ready, you can make another PR to merge the branch into master. This PR can be giant but should be OK because all changes in the branch is already reviewed. This could be an option if you think it will help with review. Note that I tagged "Design Review" for this PR which means this PR requires 3 approvals from committers for its design (user APIs, important interfaces, etc) and 1 approval for code changes. The same committers can review both design and code changes. > * These APIs are production ready (and are in fact being used in production at multiple organizations). Some of the cross-matrix options (deep storage types, sql metadata types) are more experimental (e.g. I'm unaware of any production users writing to Azure deep storage). Thanks for the answer. What is most important is not giving a wrong idea to users. Can you please document what matrix options are well tested, so that readers can get some idea about it? BTW, I don't see any new tests in .travis.yml. Unless they are running somewhere else, we should add some. > @jihoonson IMHO, for a completely new big feature like this, isn't it better to be submitted as 1 PR? If there is something wrong in the future, the git forensic would be much easier. Every PR is squashed when it is merged into master, so there is only one commit per PR in the master branch. Making some change atomic (that means, one feature is added in one PR instead of being spread across multiple PRs) can help with release, but I'm not sure how else huge PRs can make something easier.. -- 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]
