apache
[Do Not Merge] Test PR to check if pre-commit check works fine once codebase has no spotless issues
Some how 1st commit's patch doesnot apply to master. Need to check!
Code is on top of latest master, not sure why patch fails to apply with error:
16:56:40 | Vote | Subsystem | Runtime | Comment
16:56:40 ============================================================================
16:56:40 | 0 | reexec | 0m 0s | Docker mode activated.
16:56:40 | -1 | patch | 0m 6s |
16:56:40 | | | | https://github.com/apache/phoenix/pull/2120
16:56:40 | | | | does not apply to master. Rebase required?
16:56:40 | | | | Wrong Branch? See
16:56:40 | | | | https://yetus.apache.org/documentation/in-pr
16:56:40 | | | | ogress/precommit-patchnames for help.
16:56:40
Any idea @stoty ?
I think yetus simply has problems with very large patches @NihalJain .
Ok in that case we can't even have a success build for https://github.com/apache/phoenix/pull/2024 since it is huge
If yetus has problem with large patches, i wonder how we were able to make spotless change in HBase? 🤔
One way we can make progress on this is, if @NihalJain wants to re-apply spotless changes locally, followed by locally built project and also full test run and paste the result on the PR, maybe @stoty can merge the PR based on the results that Nihal posts? WDYT?
Or we can also create a feature branch and merge the changes, so that we can trigger full builds with all hbase profiles
Hi viraj sorry for delayed response. sure let me try feature branch approach. could you please help create a branch, i can rebase the changes on top of that?
Sounds good Nihal! @stoty you are fine with the feature branch approach? Let me create the branch once you confirm.
What should be the feature branch name? PHOENIX-7442-feature?
I don't think a feature branch is necessary. These are basically whitespace and import ordering changes, I don't expect any problems. IMO having mvn verify pass locally (or at least not being worse than without a patch) is enough testing.
If some problem turns up, we can fix that as we go.
IMO having mvn verify pass locally (or at least not being worse than without a patch) is enough testing.
+1, this is what i was thinking too, but having feature branch will also help validate the same on the jenkins build. So either way, it's fine: locally run full test suite or jenkins build. The feature branch build will run against all hbase profiles (nothing that should be broken specific to any profile though).
+1, this is what i was thinking too, but having feature branch will also help validate the same on the jenkins build. So either way, it's fine: locally run full test suite or jenkins build. The feature branch build will run against all hbase profiles (nothing that should be broken specific to any profile though).
OK sure let me try running locally then, although I too agree and don't expect any issues here as these are just code format changes.
Let's also agree upon a date for pushing the changes @stoty @NihalJain?
I hope the changes should be generated by spotless:apply similar to how we do in HBase, or do we need some additional steps?
I'm fine with pushing them anytime. If someone has a big patch ready, we can delay a few days to avoid having to re-do that, but I can't think of anything else to warrant a delay.
Sounds good, how about we merge PR by early next week then? And also provide guidelines on dev@phoenix to start using spotless for each PR?
Yes, we should definitely make an announcement about this. (And also update the relevant section of the website)
OK sure let me try running locally then, although I too agree and don't expect any issues here as these are just code format changes.
Ok let me take up this one now and submit a PR immediately.
Let's also agree upon a date for pushing the changes @stoty @NihalJain? I hope the changes should be generated by
spotless:applysimilar to how we do in HBase, or do we need some additional steps?
Steps remain same, no additional changes needed.
Yes, we should definitely make an announcement about this. (And also update the relevant section of the website)
Sure let us do that just before/after merging this.
Rebased https://github.com/apache/phoenix/pull/2024 and #2119; Reapplied patches here!
Will run mvn clean install -DskipTests for https://github.com/apache/phoenix/pull/2024 first. If all is well, will run mvn clean verify here.
Closing this test PR, work for this actually subsumed in https://github.com/apache/phoenix/pull/2024 and https://github.com/apache/phoenix/pull/2119