query: fix deprecation warning for tracing #1103
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
absurdfarce
reviewed
Aug 30, 2021
| @@ -996,7 +996,8 @@ def populate(self, max_wait=2.0, wait_for_complete=True, query_cl=None): | |||
| SimpleStatement(self._SELECT_SESSIONS_FORMAT, consistency_level=query_cl), (self.trace_id,), time_spent, max_wait) | |||
|
|
|||
| # PYTHON-730: There is race condition that the duration mutation is written before started_at the for fast queries | |||
| is_complete = session_results and session_results[0].duration is not None and session_results[0].started_at is not None | |||
| session_row = session_results.one() if session_results else None | |||
There was a problem hiding this comment.
Very big 👍 to moving session_row declaration higher here and using it for both the wait test and subsequent operations!
absurdfarce
reviewed
Aug 30, 2021
cassandra/query.py
Outdated
| @@ -996,7 +996,8 @@ def populate(self, max_wait=2.0, wait_for_complete=True, query_cl=None): | |||
| SimpleStatement(self._SELECT_SESSIONS_FORMAT, consistency_level=query_cl), (self.trace_id,), time_spent, max_wait) | |||
|
|
|||
| # PYTHON-730: There is race condition that the duration mutation is written before started_at the for fast queries | |||
| is_complete = session_results and session_results[0].duration is not None and session_results[0].started_at is not None | |||
| session_row = session_results.one() if session_results else None | |||
| is_complete = session_results and session_row.duration is not None and session_row.started_at is not None | |||
There was a problem hiding this comment.
It seems slightly odd to look at session_results again here. Should this perhaps be:
is_complete = session_row is not None and session_row.duration is not None and session_row.started_at is not NoneRationale is that before we knew session_results[0] (the equivalent to session_row in the new impl) was non-null since we'd already validated that session_results contained at least one row (via the boolean check in "session_results and session_results[0].duration..."). We don't have that guarantee anymore; now it's quite possible that session_row could be None.
What do you think?
There was a problem hiding this comment.
Yes, absolutely, it makes more sense to check session_row here, thanks
|
@psarna Thanks for the pull request! This is a good find; I had a minor question on the PR itself but overall this looks pretty good. Have you signed the Contributor License Agreement for contributions to DataStax open source projects? If not you can find it at https://cla.datastax.com. Thanks! |
Tracing code uses a deprecated mechanism for fetching the first row when populating traces. The behavior is now fixed.
psarna
force-pushed
the
fix_deprecation_in_tracing
branch
from
5436180 to
d0f1ec2
Compare
Signed, thanks |
|
Thanks @psarna, this looks good! |
absurdfarce
pushed a commit
that referenced
this pull request
Sep 17, 2021
This commit provides a fix to the situation when iterating on a ResultSet, the driver aborts the iteration if the server returns an empty page even if there are next pages available. Python driver is affected by the same problem as JAVA-2934 This fix is similar to apache/cassandra-java-driver#1544
fruch
added a commit
to fruch/python-driver
that referenced
this pull request
Aug 2, 2022
…to sync_with_upstream * 'master' of https://github.com/datastax/python-driver: Merge pull request datastax#1126 from eamanu/fix-typos PYTHON-1294: Upgrade importlib-metadata to a much newer version Add tests for recent addition of execution profile support to cassandra.concurrent Merge pull request datastax#1122 from andy-slac/concurrent-execution-profiles Merge pull request datastax#1119 from datastax/python-1290 Merge pull request datastax#1117 from datastax/remove_unittest2 Removing file unexpectedly included in previous PR Merge pull request datastax#1114 from haaawk/stream_ids_fix Merge pull request datastax#1116 from Orenef11/fix_default_argument_value Comment update following off of datastax#1110 Merge pull request datastax#1103 from numberly/fix_empty_paging Merge pull request datastax#1103 from psarna/fix_deprecation_in_tracing Fixes to the Travis build. (datastax#1111)
fruch
added a commit
to fruch/python-driver
that referenced
this pull request
Aug 2, 2022
…to sync_with_upstream_2 * 'master' of https://github.com/datastax/python-driver: Merge pull request datastax#1126 from eamanu/fix-typos PYTHON-1294: Upgrade importlib-metadata to a much newer version Add tests for recent addition of execution profile support to cassandra.concurrent Merge pull request datastax#1122 from andy-slac/concurrent-execution-profiles Merge pull request datastax#1119 from datastax/python-1290 Merge pull request datastax#1117 from datastax/remove_unittest2 Removing file unexpectedly included in previous PR Merge pull request datastax#1114 from haaawk/stream_ids_fix Merge pull request datastax#1116 from Orenef11/fix_default_argument_value Comment update following off of datastax#1110 Merge pull request datastax#1103 from numberly/fix_empty_paging Merge pull request datastax#1103 from psarna/fix_deprecation_in_tracing Fixes to the Travis build. (datastax#1111)
nyh
added a commit
to nyh/scylla
that referenced
this pull request
May 23, 2023
In commit 0a71151 I wanted to avoid a incorrect deprecation warning from the Python driver but fixed it in an incorrect way. I never noticed the fix was incorrect because the test was already xfailing, and the incorrect fix just made it fail differently... In this patch I revert that commit. With this revert, I am *not* bringing back the spurious warning - the Python driver bug was already fixed in datastax/python-driver#1103 - so developers with a fairly recent version will no longer see the spurious warning. Both old and new drivers will at least do the correct thing, as it was before that unfortunate commit. Fixes scylladb#8752. Signed-off-by: Nadav Har'El <[email protected]>
denesb
pushed a commit
to scylladb/scylladb
that referenced
this pull request
May 24, 2023
In commit 0a71151 I wanted to avoid a incorrect deprecation warning from the Python driver but fixed it in an incorrect way. I never noticed the fix was incorrect because the test was already xfailing, and the incorrect fix just made it fail differently... In this patch I revert that commit. With this revert, I am *not* bringing back the spurious warning - the Python driver bug was already fixed in datastax/python-driver#1103 - so developers with a fairly recent version will no longer see the spurious warning. Both old and new drivers will at least do the correct thing, as it was before that unfortunate commit. Fixes #8752. Signed-off-by: Nadav Har'El <[email protected]> Closes #14002
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tracing code uses a deprecated mechanism for fetching the first row
when populating traces. The behavior is now fixed.