Skip to content

Add option to set max_receive_message_length in client config#55

Merged
hughmiao merged 1 commit into
google:masterfrom
daikeshi:master
Sep 9, 2020
Merged

Add option to set max_receive_message_length in client config#55
hughmiao merged 1 commit into
google:masterfrom
daikeshi:master

Conversation

@daikeshi
Copy link
Copy Markdown
Contributor

@daikeshi daikeshi commented Aug 8, 2020

Fixes #42

Copy link
Copy Markdown
Contributor

@hughmiao hughmiao left a comment

Choose a reason for hiding this comment

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

thanks for the PR, Keshi. some nit comments.

Comment thread ml_metadata/proto/metadata_store.proto Outdated
Comment on lines +593 to +595

// Maximum message length that the channel can receive.
optional int64 max_receive_message_length = 4;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's create a message, e.g., GrpcChannelArguments, and use that in the MetadataStoreClientConfig as well as the MetadataStoreServerConfig too. The server needs to be aware of argument as well, correct? also having a message, helps to extend to support other channel options. wdyt?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

consider to build the custom server and try the client change to see whether this fixes your issue?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sounds good! @hughmiao, I'll work more on this PR later this week.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@hughmiao sorry for the delay. I added GrpcChannelArguments message to handle max_receive_message_length grpc option. MetadataStoreServerConfig should be fine since it's already been parsing grpc arguments (

AddGrpcChannelArgs(FLAGS_grpc_channel_arguments, &builder);
).

I also tested it locally and it worked as expected. Please take another look and let me know if it's good to go. Thank you!

In [1]: from ml_metadata.proto import metadata_store_pb2
   ...: import ml_metadata as mlmd
   ...: metadata_connection_config = metadata_store_pb2.MetadataStoreClientConfig()
   ...: metadata_connection_config.host = "127.0.0.1"
   ...: metadata_connection_config.port = 8080
   ...: metadata_connection_config.channel_arguments.max_receive_message_length = 0
   ...: store = mlmd.MetadataStore(metadata_connection_config)
   ...: store.get_artifact_types()
...
...
~/Projects/google/ml-metadata/ml_metadata/metadata_store/metadata_store.py in _call_method(self, method_name, request, response)
    172         # description.
    173         # https://grpc.github.io/grpc/python/_modules/grpc.html#StatusCode
--> 174         raise _make_exception(e.details(), e.code().value[0])  # pytype: disable=attribute-error
    175
    176   def _swig_call(self, method, request, response) -> None:

ResourceExhaustedError: Received message larger than max (3583 vs. 0)

@daikeshi daikeshi force-pushed the master branch 4 times, most recently from d6dccc7 to ec7cc1c Compare September 8, 2020 02:15
Copy link
Copy Markdown
Contributor

@hughmiao hughmiao left a comment

Choose a reason for hiding this comment

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

thanks for the contribution, @daikeshi

@hughmiao hughmiao merged commit 8cf8f0c into google:master Sep 9, 2020
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.

No way to set max receive message size in MetadataStore

2 participants