You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/10/22 10:46:58 UTC
[GitHub] [airflow] TobKed opened a new pull request #11743: Google Memcached hooks - improve protobuf messages handling
TobKed opened a new pull request #11743:
URL: https://github.com/apache/airflow/pull/11743
Use to_dict method introduced in `proto-plus 1.11.0` instead if operating on json
Related links:
https://github.com/googleapis/python-memcache/issues/19#issuecomment-713664513
https://github.com/googleapis/proto-plus-python/pull/154
https://proto-plus-python.readthedocs.io/en/latest/messages.html#serialization
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11743: Google Memcached hooks - improve protobuf messages handling
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11743:
URL: https://github.com/apache/airflow/pull/11743#discussion_r511159191
##########
File path: airflow/providers/google/cloud/hooks/cloud_memorystore.py
##########
@@ -572,7 +571,7 @@ def _append_label(instance: cloud_memcache.Instance, key: str, val: str) -> clou
@staticmethod
def proto_message_to_dict(message: proto.Message) -> dict:
"""Helper method to parse protobuf message to dictionary."""
- return json.loads(message.__class__.to_json(message))
+ return message.__class__.to_dict(message)
Review comment:
This code has been deleted. Can you look again?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11743: Google Memcached hooks - improve protobuf messages handling
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11743:
URL: https://github.com/apache/airflow/pull/11743#discussion_r510539623
##########
File path: airflow/providers/google/cloud/hooks/cloud_memorystore.py
##########
@@ -572,7 +571,7 @@ def _append_label(instance: cloud_memcache.Instance, key: str, val: str) -> clou
@staticmethod
def proto_message_to_dict(message: proto.Message) -> dict:
"""Helper method to parse protobuf message to dictionary."""
- return json.loads(message.__class__.to_json(message))
+ return message.__class__.to_dict(message)
Review comment:
Do we need this method at all? I think we can replace these methods with direct calls.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] TobKed commented on a change in pull request #11743: Google Memcached hooks - improve protobuf messages handling
Posted by GitBox <gi...@apache.org>.
TobKed commented on a change in pull request #11743:
URL: https://github.com/apache/airflow/pull/11743#discussion_r510825460
##########
File path: airflow/providers/google/cloud/hooks/cloud_memorystore.py
##########
@@ -572,7 +571,7 @@ def _append_label(instance: cloud_memcache.Instance, key: str, val: str) -> clou
@staticmethod
def proto_message_to_dict(message: proto.Message) -> dict:
"""Helper method to parse protobuf message to dictionary."""
- return json.loads(message.__class__.to_json(message))
+ return message.__class__.to_dict(message)
Review comment:
I was thinking about it as well. Please check last updates to the PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #11743: Google Memcached hooks - improve protobuf messages handling
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11743:
URL: https://github.com/apache/airflow/pull/11743#discussion_r510064505
##########
File path: airflow/providers/google/cloud/hooks/cloud_memorystore.py
##########
@@ -572,7 +571,7 @@ def _append_label(instance: cloud_memcache.Instance, key: str, val: str) -> clou
@staticmethod
def proto_message_to_dict(message: proto.Message) -> dict:
"""Helper method to parse protobuf message to dictionary."""
- return json.loads(message.__class__.to_json(message))
+ return message.__class__.to_dict(message)
Review comment:
Can you add comments in the code explaining why?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] TobKed commented on pull request #11743: Google Memcached hooks - improve protobuf messages handling
Posted by GitBox <gi...@apache.org>.
TobKed commented on pull request #11743:
URL: https://github.com/apache/airflow/pull/11743#issuecomment-714407565
PTAL @mik-laj @tanjinP
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj merged pull request #11743: Google Memcached hooks - improve protobuf messages handling
Posted by GitBox <gi...@apache.org>.
mik-laj merged pull request #11743:
URL: https://github.com/apache/airflow/pull/11743
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org