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

Changes for Python 3.10 support #107

Closed
wants to merge 1 commit into from

Conversation

dgkncelik
Copy link

Change http connection dependency
Add httpx in requirements.txt
Change api.py

Change http connection dependency
Add httpx in requirements.txt
Change api.py
@KenCox94
Copy link
Collaborator

KenCox94 commented Aug 3, 2022

@bryanyang0528 this one is critical for migration to Python 3.10.

@dgkncelik @bryanyang0528 has been working to resolve CI pipeline issues and merging pull requests. can you fix the branch conflicts? I think once we have yours we can get a new release out!

@dgkncelik
Copy link
Author

Sorry for the late response, let me correct it ASAP

@bryanyang0528 bryanyang0528 mentioned this pull request Aug 6, 2022
@bryanyang0528
Copy link
Owner

@KenCox94 I don't think I can migrate all CI processes to Gitlab Action in this days. So I add python3.10 on Travis CI temporary.
@dgkncelik Now you can test the code on travis CI

@@ -141,8 +138,8 @@ def _request2(self, endpoint, connection, body, method="POST", encoding="utf-8")
base64string = base64.b64encode(bytes("{}:{}".format(self.api_key, self.secret), "utf-8")).decode("utf-8")
headers["Authorization"] = "Basic %s" % base64string

connection.request(method=method.upper(), url=url, headers=headers, body=data)
resp = connection.get_response()
resp = connection(method=method.upper(), url=url, headers=headers, body=data)
Copy link

@cordawyn cordawyn Aug 16, 2022

Choose a reason for hiding this comment

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

Sorry for my 5 cents, but I tried testing queries with HTTP/2 and found an issue which lead here. Connection (which is httpx.request object here) does not want body, but content argument instead. It looks like more refactoring is in order.

print("Ending query because of time out! ({} seconds)".format(idle_timeout))
return
else:
raise ValueError("Return code is {}.".format(streaming_response.status))

Choose a reason for hiding this comment

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

Should be streaming_response.status_code?

@KenCox94
Copy link
Collaborator

@cordawyn thank you for bringing this up.
@dgkncelik are there any updates on the conflicts? Also, do you think you can correspond with @cordawyn to resolve the issues he mentioned above?
The quicker this gets resolved, the sooner we can move to a new release.

@bryanyang0528

@cordawyn
Copy link

cordawyn commented Aug 19, 2022

@KenCox94 with all the changes I suggested above, I made it to the point where it tries to get the query response, I guess. Here's the error I get now:

{"@type":"statement_error","error_code":40001,"message":"Error starting pull query: Cannot determine which host contains the required partitions to serve the pull query. \nThe underlying persistent query may be restarting (e.g. as a result of ALTER SYSTEM) view the status of your by issuing <DESCRIBE foo>.\nStatement: SELECT * FROM ksql_test_table_1;: SELECT * FROM ksql_test_table_1;","statementText":"SELECT * FROM ksql_test_table_1;","entities":[]}

I remember I used to get this error when these env variables for Kafka were not set as suggested:

  • transaction.state.log.replication.factor: 1
  • transaction.state.log.min.isr: 1
  • offsets.topic.replication.factor: 1

... but they are properly set now. Could be another reason I guess.

The query itself is created in KSQL and I can "select" from it using ksql CLI. It worked fine in ksql-python with non-HTTP/2 querying though.

@KenCox94
Copy link
Collaborator

OK, here is where I am at with this..........this API will NOT WORK w/ python >=3.10 if we don't implement some of these changes.

So, @dgkncelik, If we don't hear anything back concerning the resolution of the branch conflicts within 48 hours I am going to close this pull request, make the changes you implemented, and open it as a new pull request. Please don't take this as me being terse but we have been sitting on this for a while now. The lapse isn't just on you; I understand that, but things have to get moving.

@cordawyn @bryanyang0528 this pull request title is so encompassing that we really need to make this more of an epic. There isn't going to be a catch-all pull request to settle all problems with 3.10 as @cordawyn has demonstrated.

@cordawyn could you take on what you have put forth? Just fix and submit a new pull request based on those. If not, could you submit these as issues so we remember these need addressed?

I think it's important to get this request pushed through as it's such a simple fix but it will be renamed.

Please let me know your thoughts on this and apologies for the sabbatical. I will be continuing some more aggressive work on this until the holidays here in the US.

@KenCox94
Copy link
Collaborator

Closing will open a new pull request.

@Robbie-Palmer
Copy link

@KenCox94, any word on a new PR for migrating this library from hyper to httpx?
I have just opened an issue that requires the httpx migration #120

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.

None yet

5 participants