Contributing update review tips #7107
Conversation
Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>
acarbonetto
requested review from
reta,
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
gbbafna,
setiah,
kartg,
kotwanikunal,
mch2,
nknize,
owaiskazi19,
Rishikesh1159,
ryanbogan,
saratvemulapalli,
shwetathareja,
dreamer-89,
tlfeng,
VachaShah and
xuezhou25
as code owners
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>
There was a problem hiding this comment.
Should this be in https://github.com/opensearch-project/OpenSearch/blob/main/DEVELOPER_GUIDE.md actually?
That's a good question. I could move it, but I thought it might be best here as is summarizes what is already in the DEVELOPER_GUIDE in a way that highlights best practices and ways to make your PR get accepted. But I'm open to moving it if you don't think this is the place for a summary? |
Gradle Check (Jenkins) Run Completed with:
|
|
@acarbonetto Yeah I think that would be best because we copy CONTRIBUTING.md for general high level stuff across all repos. You'll get it read more often if it's int he developer guide. Sorry to create more work for you! |
|
This PR is stalled because it has been open for 30 days with no activity. Remove stalled label or comment or this will be closed in 7 days. |
|
This PR was closed because it has been stalled for 7 days with no activity. |
|
Apologies. This PR was auto closed without reaching a resolution from the maintainers. |
kotwanikunal
requested review from
peternied,
dbwiddis,
sachinpkale,
sohami and
msfroh
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7107 +/- ##
============================================
- Coverage 71.10% 70.68% -0.43%
+ Complexity 59808 59441 -367
============================================
Files 4852 4852
Lines 285138 285138
Branches 41107 41107
============================================
- Hits 202755 201550 -1205
- Misses 66040 67036 +996
- Partials 16343 16552 +209 ☔ View full report in Codecov by Sentry. |
Compatibility status:Checks if related components are compatible with change a9ff576 Incompatible componentsSkipped componentsCompatible components |
Gradle Check (Jenkins) Run Completed with:
|
|
|
||
| We have a lot of mechanisms to help expedite towards an accepted PR. Here are some tips for success: | ||
| 1. *Minimize BWC guarantees*: First PR review heavily focuses on the public facing API. This is what we have to "guarantee" as non-breaking for bwc across major versions. | ||
| 2. *Do not copy non-complient code*: Ensure that code is APLv2 compatible. This means that you have not copied any code from other sources unless that code is also APLv2 compatible. |
There was a problem hiding this comment.
complient compliant
| We have a lot of mechanisms to help expedite towards an accepted PR. Here are some tips for success: | ||
| 1. *Minimize BWC guarantees*: First PR review heavily focuses on the public facing API. This is what we have to "guarantee" as non-breaking for bwc across major versions. | ||
| 2. *Do not copy non-complient code*: Ensure that code is APLv2 compatible. This means that you have not copied any code from other sources unless that code is also APLv2 compatible. | ||
| 3. *Use feature flags*: New features that are guarded behind a feature flag have a higher chance of being merged and backported since... they're guarded by feature flag ([Feature PR](https://github.com/opensearch-project/OpenSearch/pull/4959)). |
There was a problem hiding this comment.
How about also linking the https://github.com/opensearch-project/OpenSearch/blob/main/DEVELOPER_GUIDE.md#experimental-development section here?
| @@ -162,3 +162,18 @@ During the PR process, expect that there will be some back-and-forth. Please try | |||
| If we accept the PR, a [maintainer](MAINTAINERS.md) will merge your change and usually take care of backporting it to appropriate branches ourselves. | |||
|
|
|||
| If we reject the PR, we will close the pull request with a comment explaining why. This decision isn't always final: if you feel we have misunderstood your intended change or otherwise think that we should reconsider then please continue the conversation with a comment on the PR and we'll do our best to address any further points you raise. | |||
|
|
|||
| We have a lot of mechanisms to help expedite towards an accepted PR. Here are some tips for success: | |||
| 1. *Minimize BWC guarantees*: First PR review heavily focuses on the public facing API. This is what we have to "guarantee" as non-breaking for bwc across major versions. | |||
There was a problem hiding this comment.
We should also link https://github.com/opensearch-project/OpenSearch/blob/main/DEVELOPER_GUIDE.md#backwards-compatibility here for details on this front
| 6. *Micro-benchmark critical path*: This is a lesser known mechanism, but if you have critical path changes you're afraid will impact performance (gc, heap, direct memory, CPU) then including a [microbenchmark](https://github.com/opensearch-project/OpenSearch/tree/main/benchmarks) with your PR (and jfr or flamegraph results in the description) is a *GREAT IDEA* and will help expedite the review process. | ||
| 7. *test, test, test*: pretty self explanatory ([OpenSearchTestCase](./test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java) for unit tests, [OpenSearchIntegTestCase](./test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java) for integration & cluster tests, [OpenSearchRestTestCase](./test/framework/src/main/java/org/opensearch/test/rest/OpenSearchRestTestCase.java) for testing REST endpoint interfaces, and yaml tests with [ClientYamlTestSuiteIT](./rest-api-spec/src/yamlRestTest/java/org/opensearch/test/rest/ClientYamlTestSuiteIT.java) for REST integration tests) | ||
|
|
||
| If you're bias towards liberal guardrails, you have a higher chance of the PR getting merged quickly. We can always relax these guard rails in smaller followup PRs. Reverting a GA feature is much more difficult. Check out the [DEVELOPER_GUIDE](./DEVELOPER_GUIDE.md#submitting-changes) for more useful tips. |
There was a problem hiding this comment.
If you're bias towards liberal guardrails ...
Should this be "If you are biased towards restrictive guardrails ..." instead?
|
Hi @acarbonetto, Is this being worked upon? Feel free to reach out to maintainers for further reviews. |
Hi @ticheng-aws - sorry I've been off project for a while. I'll take a look in day or two if nobody else does. |
ticheng-aws
added
the
enhancement
|
This PR is stalled because it has been open for 30 days with no activity. |
opensearch-trigger-bot
bot
added
stalled
|
I opened a PR to get this over the finish line. Thanks for all the work @acarbonetto. At this point we can close this. @peternied could you help there? Thanks |
|
I can close this one @scrawfor99! Please refer to #12981 for further updates. |
Description
Update contributing page to include tips/tricks to help developers get their PRs into an approved state (as discussed here: https://opensearch.slack.com/archives/C038PDJ14JZ/p1681232261749639?thread_ts=1681231289.117129&cid=C038PDJ14JZ)
Issues Resolved
none
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.