Skip to content
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

Sync with upstream up to 3.29.1 #329

Draft
wants to merge 66 commits into
base: master
Choose a base branch
from

Conversation

fruch
Copy link

@fruch fruch commented Jun 6, 2024

doing it slow one version at a time

  • 3.27.0
  • 3.28.0
  • 3.29.0
  • 3.29.1

absurdfarce and others added 30 commits March 27, 2023 20:22
* update RH nav order

* add line break

* add api
Added error handling blog reference.
@fruch fruch force-pushed the sync_with_upstream_3.29.1 branch 13 times, most recently from 5e09863 to 9386a78 Compare June 10, 2024 20:39
windows builds so far was running with having libev available
and until this sync the fallback for python 3.12 was asyncio
eventloop, but now we fail and not fall back to asyncio.
so all unittest on windows are failing on any import from
cassandra.connection.

in this change we use conan to download libev, and using
it to compile the driver with libev

Ref: https://conan.io/center/recipes/libev
we don't have libev availble for this platform,
and we can run the unit tests anymore for python 3.12 cause of that.

so whom still need this platform whould need to be able to compile
it on he's own.
@fruch fruch force-pushed the sync_with_upstream_3.29.1 branch from 9386a78 to 93f12d3 Compare June 10, 2024 21:12
@fruch
Copy link
Author

fruch commented Jun 10, 2024

@Lorak-mmk

Took me a while to get the windows wheel building in shape
it was failing in testing with python 3.12, cause of missing libev (i.e. no fallback to asyncio anymore)
I've figured out a way to retrieve a compiled version of libev, but only for win64, not with win32
I'm giving up on wheels for win32, i.e. wheels we can't cover with tests are not really someone we can consider supported

(we have more like that, all the PyPy ones, but that's a fight for another day)

the only pending issue here, is getting agreement on how to treat Column Encryption feature.
and get it reviewed

@fruch
Copy link
Author

fruch commented Jun 11, 2024

One more issue I remember we might have
scylladb/scylla-cqlsh#90

cqlsh would break with python 3.12

@fruch fruch requested a review from roydahan June 20, 2024 21:51
@Lorak-mmk Lorak-mmk self-assigned this Jul 9, 2024
@Lorak-mmk
Copy link

That's a lot of code to review... I'll try to get to it soon.
Do you think it would be possible to create an automation to open a PR to our fork whenever a PR is merged to upstream (or non-PR commits are added to master in upstream)?
That way we could review the changes as they come instead of dealing with those insane massive diffs.

@fruch
Copy link
Author

fruch commented Jul 13, 2024

That's a lot of code to review... I'll try to get to it soon.
Do you think it would be possible to create an automation to open a PR to our fork whenever a PR is merged to upstream (or non-PR commits are added to master in upstream)?
That way we could review the changes as they come instead of dealing with those insane massive diffs.

We might consider that, but it would be a bit complicated, and we'll need to make sure we merge them in the right order.

It might be easy if we do it per release, in this PR we have 3 releases at the same time.

@Lorak-mmk
Copy link

@Lorak-mmk we need to take a call here, on 3.27.0 they introduce client side Column Encryption feature.

Meanwhile I don't mention it on our docs, and we should consider if to disable it in our fork (at least until we review/test it and gives it a thumbs up)

I don't think we've seen new feature much since we forked...

@roydahan @tzach, your thoughts about how to handle this feature ?

Are there tests for it?

README.rst Outdated
@@ -7,7 +7,7 @@ DataStax Driver for Apache Cassandra
A modern, `feature-rich <https://github.com/datastax/python-driver#features>`_ and highly-tunable Python client library for Apache Cassandra (2.1+) and
DataStax Enterprise (4.7+) using exclusively Cassandra's binary protocol and Cassandra Query Language v3.

The driver supports Python 2.7, 3.5, 3.6, 3.7 and 3.8.
The driver supports Python 3.7 and 3.8.

Choose a reason for hiding this comment

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

I think we support more versions than that. Could you add the missing ones?

docs/index.rst Outdated
The driver supports Python 2.7, 3.5, 3.6, 3.7 and 3.8.
The driver supports Python 3.7 and 3.8.

Choose a reason for hiding this comment

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

ditto

Comment on lines +2570 to +2573
self.client_protocol_handler = type(
str(self.session_id) + "-ProtocolHandler",
(ProtocolHandler,),
{"column_encryption_policy": self.cluster.column_encryption_policy})

Choose a reason for hiding this comment

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

Why would someone ever write such a code. It's absolutely disgusting.

Choose a reason for hiding this comment

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

This is a message reharding commit PYTHON-1313 Fix asyncio removals in Python 3.10 (https://github.com/datastax/python-driver/pull/1179):
I think this commit should be dropped, we already fixed this issue on our end.

another thing: why does the diff I see not include our changes? Is that a GitHub issue or a just how git works? It looks like this:
image
but our current code on master is different than red parts imply.
I'll stop the review here for now, because I don't see a point in reviewing if the diffs I see are false. What can we do about this?

Choose a reason for hiding this comment

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

Hmm... the diff is correct if I look in a "total" diff, but wrong if I review commit-by-commit (as reviews should be done!). I don't want to review the total diff because it includes my changes to remove six and those diffs are obfuscating the whole thing too much for me to be sure I noticed everything else.

Choose a reason for hiding this comment

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

One solution would be to fix and merge my PR with six removal and then rebase this one on top of that. This is still not a good solution because it won't allow me to review each commit, but it would be better than current state.

This situation convinces me a bit more that merging upstream PR by PR (commit by commit?) is a better solution, because it would basically avoid this problem entirely.

Copy link

@Lorak-mmk Lorak-mmk Jul 16, 2024

Choose a reason for hiding this comment

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

I rebased #243 on master and applied fixes that were done in upstream.

Copy link
Author

Choose a reason for hiding this comment

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

So you want to redo this PR, one commit at a time ?

it's not that it's easily to rebase this one, it's means to redo it.
I would suggest doing a PR per version, and then pulling in the fixes as needed from the commits I have ontop of the merges.

Choose a reason for hiding this comment

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

No, I think we should do this as one big PR (or a PR per version), as it is now.
I can try to redo this one.

Copy link
Author

Choose a reason for hiding this comment

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

o.k. lets do that.

we can automate the release part, and opening PRs for new releases, I think it would be fairly easy
to make a schedule like that

@Lorak-mmk Lorak-mmk removed their assignment Jul 31, 2024
@dkropachev dkropachev self-assigned this Aug 12, 2024
@dkropachev dkropachev marked this pull request as draft August 12, 2024 17:54
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.