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

document memoryview usage, minor frame_writer.write_frame refactor #384

Merged
merged 3 commits into from
Dec 24, 2021

Conversation

pawl
Copy link
Contributor

@pawl pawl commented Dec 20, 2021

I was looking at more py-amqp code and found a really interesting usage of memoryview in frame_writer's buffer_store. I added some comments explaining why it's used.

I also moved some variables in frame_writer's write_frame closer to where they're actually used. The buffer_store isn't used by write_frame when it ends up in the if bigbody: block, so I moved the code related to the buffer_store into the else:.

@pawl pawl changed the title document memory_view usage, minor frame_writer.write_frame refactor document memoryview usage, minor frame_writer.write_frame refactor Dec 20, 2021
@pawl
Copy link
Contributor Author

pawl commented Dec 20, 2021

The integration tests failed to pull the rabbitmq image from dockerhub:

Step 1/6 : FROM rabbitmq:latest
Head "https://registry-1.docker.io/v2/library/rabbitmq/manifests/latest": received unexpected HTTP status: 503 Service Unavailable
Error: Process completed with exit code 1.

I think they'll pass if the tests are run again later.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

please make sure unit & integration tests are added or updated to avoid regression

@auvipy
Copy link
Member

auvipy commented Dec 21, 2021

errors  
0  
code "UNAUTHORIZED"
message "authentication required"
detail  
0  
Type "repository"
Class ""
Name "library/rabbitmq"
Action "pull"

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

the unit tests look good, can you please try to add some integration test as well? besides i want to read the links thoroughly and want others try the open patches so that the don't break

@pawl
Copy link
Contributor Author

pawl commented Dec 21, 2021

@auvipy I added that integration test here: 98b8bbc

@auvipy auvipy merged commit 892389c into celery:master Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants