thempatel commented on pull request #16961: URL: https://github.com/apache/beam/pull/16961#issuecomment-1085806815
> So the vast majority of Python changes seem to be unrelated to this (mostly go specific, already very large) change. Is there a way we could break them out and review them separately? I sympathize with this, it's usually good advice to break large changes into their logical components and land them separately. I think in this case, that will be a challenge; I see the change-set here as one logical component. The nature of building a polyglot SDK on top of proto is that if you change the proto, you have to change all the languages and there's really no way around this without some long migration path. To that end, I think the per-language changes here are isolated enough where you can likely hide the changes from other files and review just the python files, a similar experience to what it would be like if we housed them in their own PR. Maybe a question to ask is what we do if something inexplicably goes wrong with merging this PR: is it easier to recover quickly if we have this change set spread over multiple commits, or a single commit? A revert of 1 commit is easy, a revert of multiple commits with others interspersed is much harder. If you feel really stron gly that the python changes should be in their own change set, I am happy to oblige. >The one change I see is that we (now) have to do renaming of the proto files in apache_beam/portability/api back to a flat structure (and fix their internal imports). Let's chat about this, I'm not sure I understand what is being said here. Why would we have to change this back to a flat structure? I think that will be impossible, hence the extensive changes in the generation tooling. > Also, for auto-generated files, I think adding them to RAT is better than modifying these files--they're (clearly marked) machine output. It looks like the generated go files are already in the RAT, though I do agree with Robert that maintaining the ASF license header is a better avenue, plus it reduces the diff on the PR -- 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]
