-
Notifications
You must be signed in to change notification settings - Fork 557
PYTHON-1402 Support running test suite with HCD 1.0.0 #1234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Kicked off a Jenkins build of this to confirm we're okay, will post updates here when that completes. |
Bah, that doesn't make any sense... we won't use this Jenkinsfile, and without that we don't get testing on the new platforms. Might wind up just merging these fixes and cleaning up anything which still looks out of whack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks quite good! Think we still need a bit more work re: how we include HCD versions into the existing set of matrices but that's the only thing we need to do at this point.
Jenkinsfile
Outdated
"SERVER": DEFAULT_HCD, | ||
"RUNTIME": DEFAULT_RUNTIME, | ||
"CYTHON": DEFAULT_CYTHON | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed last week but the default matrix used for most test builds is the "SMOKE" matrix... and it seems to me we'd prefer to bring HCD into that matrix rather than introducing a new matrix for HCD (which would have to be explicitly triggered). To that end this PR should:
- Remove the "HCD" matrix above
- Either add HCD versions to the DEFAULT_DSE array or keep them in the DEFAULT_HCD array
- Update the server selection logic below (or here) to include HCD versions in some way.
There is some subtlety here. We're already pulling the most recent DSE version for smoke tests via the DEFAULT_DSE.takeRight(1)
call. What we probably want to wind up with is smoke tests running against the current DSE minor (6.9.x), the previous DSE minor (6.8.x) and HCD. You can make this happen whether you keep a separate array for HCD or fold it into the DEFAULT_DSE array.
Keep in mind that every new server backend adds 2 x (number of Python versions) builds to the Jenkins runs... so we want to be judicious about what we add here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good catch. Fixed.
@@ -942,7 +942,7 @@ def test_no_connection_refused_on_timeout(self): | |||
exception_type = type(result).__name__ | |||
if exception_type == "NoHostAvailable": | |||
self.fail("PYTHON-91: Disconnected from Cassandra: %s" % result.message) | |||
if exception_type in ["WriteTimeout", "WriteFailure", "ReadTimeout", "ReadFailure", "ErrorMessageSub"]: | |||
if exception_type in ["WriteTimeout", "WriteFailure", "ReadTimeout", "ReadFailure", "ErrorMessageSub", "ErrorMessage"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know why we're suddenly seeing the parent class ErrorMessage
here when we weren't before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Driver receives exceptions:
WriteTimeout('Error from server: code=1100 [Coordinator node timed out waiting for replica nodes\' responses] message="CAS operation timed out - encountered contentions: 18" info={\'consistency\': \'SERIAL\', \'required_responses\': 2, \'received_responses\': 0, \'write_type\': \'CAS\'}')
ErrorMessage: <Error from server: code=1700 [Unknown] message="CAS operation result is unknown - proposal accepted by 1 but not a quorum.">
It looks clear here why CAS_WRITE_UNKNOWN
is wrapped in ErrorMessage
. Cannot answer why this was not the case before.
I have asked HCD engineers around, but they are not familiar with LWT changes introduced in C* 5.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking into it. For the record I don't think this difference should hold up getting this PR in. It seemed like an odd change when looking this over but I can envision perfectly good reasons why it might be the case.
@@ -51,7 +53,7 @@ matrices = [ | |||
"CYTHON": DEFAULT_CYTHON | |||
], | |||
"SMOKE": [ | |||
"SERVER": DEFAULT_CASSANDRA.takeRight(2) + DEFAULT_DSE.takeRight(1), | |||
"SERVER": DEFAULT_CASSANDRA.takeRight(2) + DEFAULT_DSE.takeRight(2) + DEFAULT_HCD.takeRight(1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record: I had this wrong in my earlier comment. We should actually be adding only four new builds to our smoke test build matrix with this change: 2 new backends * 2 Python versions (oldest and newest). That should be reasonably manageable. More to the point it's hard to imagine how we'd remove either 6.8.x, 6.9.x or HCD from our test matrix.
No description provided.