Skip to content

Conversation

@mcvsubbu
Copy link
Contributor

@mcvsubbu mcvsubbu commented Oct 9, 2020

This PR includes a basic framework for us to build compatibility tests across releases
The actual tests and a lot of code is yet to be written.

The framework is set up so that we can run operations during a cluster upgrade, and
detect incompatibility across releases. The value of the tests will be in the operations
we choose to test during upgrades

Configuration is done via YAML, and some sample yaml files and skinny classes have been
provided.

Issue 4854

Description

Add a description of your PR here.
A good description should include pointers to an issue or design document, etc.

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

  • Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

  • Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

  • New configuration options
  • Deprecation of configurations
  • Signature changes to public methods/interfaces
  • New plugins added or old plugins removed
  • Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release.

If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text

Documentation

If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document

This PR includes a basic framework for us to build compatibility tests across releases
The actual tests and a lot of code is yet to be written.

The framekwork is set up so that we can run operations during a cluster upgrade, and
detect incompatibility across releases. The value of the tests will be in the operations
we choose to test during upgrades

Configuration is done via YAML, and some sample yaml files and skinny classes have been
provided.
@codecov-io
Copy link

codecov-io commented Oct 10, 2020

Codecov Report

Merging #6129 into master will increase coverage by 6.41%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6129      +/-   ##
==========================================
+ Coverage   66.44%   72.86%   +6.41%     
==========================================
  Files        1075     1223     +148     
  Lines       54773    57744    +2971     
  Branches     8168     8522     +354     
==========================================
+ Hits        36396    42075    +5679     
+ Misses      15700    12912    -2788     
- Partials     2677     2757      +80     
Flag Coverage Δ
#integration 45.44% <48.40%> (?)
#unittests 64.01% <37.80%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ot/broker/broker/AllowAllAccessControlFactory.java 100.00% <ø> (ø)
.../helix/BrokerUserDefinedMessageHandlerFactory.java 52.83% <0.00%> (-13.84%) ⬇️
...ava/org/apache/pinot/client/AbstractResultSet.java 53.33% <0.00%> (-3.81%) ⬇️
.../main/java/org/apache/pinot/client/Connection.java 44.44% <0.00%> (-4.40%) ⬇️
.../org/apache/pinot/client/ResultTableResultSet.java 24.00% <0.00%> (-10.29%) ⬇️
...not/common/assignment/InstancePartitionsUtils.java 78.57% <ø> (+5.40%) ⬆️
.../apache/pinot/common/exception/QueryException.java 90.27% <ø> (+5.55%) ⬆️
...pinot/common/function/AggregationFunctionType.java 98.27% <ø> (-1.73%) ⬇️
.../pinot/common/function/DateTimePatternHandler.java 83.33% <ø> (ø)
...ot/common/function/FunctionDefinitionRegistry.java 88.88% <ø> (+44.44%) ⬆️
... and 970 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2379791...a251ee7. Read the comment docs.

Comment on lines 160 to 184
sleep 20
#$COMPAT_TESTER pre-controller-upgrade.yaml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why 20 seconds sleep was chosen in the first place, but does replacing it with running compat_tester mean the tests are going to take 20 seconds?
Also maybe it reads better if $COMPAT_TESTER is replaced with a function call, something like runCompatiblityTests, which is more consistent with startService, startService, ... here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the sleep. It should not be needed.
Replacing with a function call may be fine, but we still have to check the return from that function. I have added the status check now. Let it be like this for now, until we add more meat to it

Comment on lines 204 to 205
#$COMPAT_TESTER post-controller-rollback.yaml
sleep 20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you decide to have(or remove) previous sleeps, please do the same for this one as well?

Copy link
Member

@jackjlli jackjlli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor but LGTM

boolean runOp() {
switch(_op) {
case UPLOAD:
System.out.println("Generating segment " + _segmentName + " from " + _inputDataFileName + " and uploading to " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may not need to print it out in the console?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will not need it. I put it there so we can see what is being run. Once we add more logic here, we can remove these things

@mcvsubbu
Copy link
Contributor Author

we should remove all sleeps. They were added by the earlier committer. I will do that.
We also need to check the status of each compat-tester callout.
Lastly, I think this whole thing needs to move into pinot-integration-tests, so that we can re-use all the code in there in the compat test operations. What do you say?

@mcvsubbu mcvsubbu changed the title Issue #4854 Framework for adding compatibility tests Framework for adding compatibility tests Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants