You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "asosnovsky (via GitHub)" <gi...@apache.org> on 2023/07/05 15:34:27 UTC
[GitHub] [avro] asosnovsky opened a new pull request, #2319: AVRO-0: Python patched up ipc issues
asosnovsky opened a new pull request, #2319:
URL: https://github.com/apache/avro/pull/2319
## What is the purpose of the change
- Patched issue where FramedReader resulted in an infinite loop where it cannot read anything from the response (it was comparing bytes to a string)
- Patched error handling for the Responder:
- Invalid error source: `except` was pointing at `schema.AvroException` instead of `avro.errors.AvroException`
- "Handshake" was lost for cases where the response could not be read due to things like "type errors"
- Added some types to responder class
## Verifying this change
This change added tests and can be verified as follows:
- New test cases added
- Type checker for Responder works (while we have tested it in our repo, I am honestly a bit confused on how to setup the local server with this code base)
## Documentation
- Does this pull request introduce a new feature? NO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on pull request #2319: AVRO-0: Python patched up ipc issues
Posted by "kojiromike (via GitHub)" <gi...@apache.org>.
kojiromike commented on PR #2319:
URL: https://github.com/apache/avro/pull/2319#issuecomment-1641075189
I think this is good, but can we attach it to [any existing tickets](https://issues.apache.org/jira/browse/AVRO-1639?jql=project%20%3D%20AVRO%20AND%20text%20~%20ipc%20and%20statusCategory%20!%3D%20done%20and%20component%20%3D%20python)? If not, I can open a new one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on pull request #2319: AVRO-3803: Python patched up ipc issues
Posted by "kojiromike (via GitHub)" <gi...@apache.org>.
kojiromike commented on PR #2319:
URL: https://github.com/apache/avro/pull/2319#issuecomment-1642907678
@asosnovsky could I trouble you to check if this implementation resolves [AVRO-1639](https://issues.apache.org/jira/browse/AVRO-1639)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] asosnovsky commented on a diff in pull request #2319: AVRO-0: Python patched up ipc issues
Posted by "asosnovsky (via GitHub)" <gi...@apache.org>.
asosnovsky commented on code in PR #2319:
URL: https://github.com/apache/avro/pull/2319#discussion_r1267092003
##########
lang/py/avro/ipc.py:
##########
@@ -382,7 +457,7 @@ def read_framed_message(self):
return b"".join(message)
while buffer.tell() < buffer_length:
chunk = self.reader.read(buffer_length - buffer.tell())
- if chunk == "":
+ if chunk == b"":
Review Comment:
yes it is
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] asosnovsky-sumologic commented on a diff in pull request #2319: AVRO-0: Python patched up ipc issues
Posted by "asosnovsky-sumologic (via GitHub)" <gi...@apache.org>.
asosnovsky-sumologic commented on code in PR #2319:
URL: https://github.com/apache/avro/pull/2319#discussion_r1267158454
##########
lang/py/avro/test/test_ipc.py:
##########
@@ -38,6 +40,12 @@ def test_server_with_path(self):
client_with_default_path = avro.ipc.HTTPTransceiver("apache.org", 80)
self.assertEqual("/", client_with_default_path.req_resource)
+ def test_empty_reader(self):
+ response_reader = avro.ipc.FramedReader(io.BytesIO(b"Bad Response"))
+ with self.assertRaises(avro.errors.ConnectionClosedException) as cm:
+ response_reader.read_framed_message()
+ assert str(cm.exception) == "Reader read 0 bytes."
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on pull request #2319: AVRO-3803: Python patched up ipc issues
Posted by "kojiromike (via GitHub)" <gi...@apache.org>.
kojiromike commented on PR #2319:
URL: https://github.com/apache/avro/pull/2319#issuecomment-1643040033
As far as resolving the conflicts: If you take your branch as-is and run `autoflake` on it, that should be sufficient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on a diff in pull request #2319: AVRO-0: Python patched up ipc issues
Posted by "kojiromike (via GitHub)" <gi...@apache.org>.
kojiromike commented on code in PR #2319:
URL: https://github.com/apache/avro/pull/2319#discussion_r1267105398
##########
lang/py/avro/test/test_ipc.py:
##########
@@ -38,6 +40,12 @@ def test_server_with_path(self):
client_with_default_path = avro.ipc.HTTPTransceiver("apache.org", 80)
self.assertEqual("/", client_with_default_path.req_resource)
+ def test_empty_reader(self):
+ response_reader = avro.ipc.FramedReader(io.BytesIO(b"Bad Response"))
+ with self.assertRaises(avro.errors.ConnectionClosedException) as cm:
+ response_reader.read_framed_message()
+ assert str(cm.exception) == "Reader read 0 bytes."
Review Comment:
I think checking the exact message is unnecessary. Testing for the expected exception type should be sufficient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] asosnovsky commented on pull request #2319: AVRO-0: Python patched up ipc issues
Posted by "asosnovsky (via GitHub)" <gi...@apache.org>.
asosnovsky commented on PR #2319:
URL: https://github.com/apache/avro/pull/2319#issuecomment-1641271007
@kojiromike no I don't have a JIRA for this. Please open one :) thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] asosnovsky commented on a diff in pull request #2319: AVRO-0: Python patched up ipc issues
Posted by "asosnovsky (via GitHub)" <gi...@apache.org>.
asosnovsky commented on code in PR #2319:
URL: https://github.com/apache/avro/pull/2319#discussion_r1267091036
##########
lang/py/avro/ipc.py:
##########
@@ -382,7 +457,7 @@ def read_framed_message(self):
return b"".join(message)
while buffer.tell() < buffer_length:
chunk = self.reader.read(buffer_length - buffer.tell())
- if chunk == "":
+ if chunk == b"":
Review Comment:
yes it is
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on a diff in pull request #2319: AVRO-0: Python patched up ipc issues
Posted by "kojiromike (via GitHub)" <gi...@apache.org>.
kojiromike commented on code in PR #2319:
URL: https://github.com/apache/avro/pull/2319#discussion_r1264273968
##########
lang/py/avro/ipc.py:
##########
@@ -382,7 +457,7 @@ def read_framed_message(self):
return b"".join(message)
while buffer.tell() < buffer_length:
chunk = self.reader.read(buffer_length - buffer.tell())
- if chunk == "":
+ if chunk == b"":
Review Comment:
Isn't it the same bug on line 467?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on a diff in pull request #2319: AVRO-0: Python patched up ipc issues
Posted by "kojiromike (via GitHub)" <gi...@apache.org>.
kojiromike commented on code in PR #2319:
URL: https://github.com/apache/avro/pull/2319#discussion_r1264272047
##########
lang/py/avro/schema.py:
##########
@@ -873,20 +873,37 @@ def __init__(
if schema_type == "request":
Schema.__init__(self, schema_type, other_props)
else:
- NamedSchema.__init__(self, schema_type, name, namespace, names, other_props, validate_names=validate_names)
+ NamedSchema.__init__(
+ self,
+ schema_type,
+ name,
+ namespace,
+ names,
+ other_props,
+ validate_names=validate_names,
+ )
names = names or Names(validate_names=self.validate_names)
- if schema_type == "record":
+ if (schema_type == "record") or (schema_type == "error"):
Review Comment:
```suggestion
if schema_type in ("record", "error"):
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] kojiromike commented on a diff in pull request #2319: AVRO-0: Python patched up ipc issues
Posted by "kojiromike (via GitHub)" <gi...@apache.org>.
kojiromike commented on code in PR #2319:
URL: https://github.com/apache/avro/pull/2319#discussion_r1264272206
##########
lang/py/avro/schema.py:
##########
@@ -873,20 +873,37 @@ def __init__(
if schema_type == "request":
Schema.__init__(self, schema_type, other_props)
else:
- NamedSchema.__init__(self, schema_type, name, namespace, names, other_props, validate_names=validate_names)
+ NamedSchema.__init__(
+ self,
+ schema_type,
+ name,
+ namespace,
+ names,
+ other_props,
+ validate_names=validate_names,
+ )
names = names or Names(validate_names=self.validate_names)
- if schema_type == "record":
+ if (schema_type == "record") or (schema_type == "error"):
old_default = names.default_namespace
- names.default_namespace = Name(name, namespace, names.default_namespace, validate_name=validate_names).space
+ names.default_namespace = Name(
+ name,
+ namespace,
+ names.default_namespace,
+ validate_name=validate_names,
+ ).space
# Add class members
- field_objects = RecordSchema.make_field_objects(fields, names, validate_names=validate_names)
+ field_objects = RecordSchema.make_field_objects(
+ fields,
+ names,
+ validate_names=validate_names,
+ )
self.set_prop("fields", field_objects)
if doc is not None:
self.set_prop("doc", doc)
- if schema_type == "record":
+ if (schema_type == "record") or (schema_type == "error"):
Review Comment:
```suggestion
if schema_type in ("record", "error"):
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] asosnovsky commented on a diff in pull request #2319: AVRO-0: Python patched up ipc issues
Posted by "asosnovsky (via GitHub)" <gi...@apache.org>.
asosnovsky commented on code in PR #2319:
URL: https://github.com/apache/avro/pull/2319#discussion_r1267091036
##########
lang/py/avro/ipc.py:
##########
@@ -382,7 +457,7 @@ def read_framed_message(self):
return b"".join(message)
while buffer.tell() < buffer_length:
chunk = self.reader.read(buffer_length - buffer.tell())
- if chunk == "":
+ if chunk == b"":
Review Comment:
yes it is
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org