You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2021/11/11 17:52:39 UTC
[GitHub] [beam] lukecwik commented on a change in pull request #15917: [BEAM-13193] Add elements field in ProcessBundleRequest and ProcessBu…
lukecwik commented on a change in pull request #15917:
URL: https://github.com/apache/beam/pull/15917#discussion_r747702753
##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -327,6 +335,13 @@ message ProcessBundleResponse {
// payload field with the bytes specified here.
map<string, bytes> monitoring_data = 5;
+ // (optional) Output elements of the processed bundle. Either all or
+ // none of the bundle elements should be included in the ProcessBundleResponse.
+ // This elements embedding is to achieve better Fn API efficiency for bundles
+ // that only produce small amount of outputs. See more at
+ // https://s.apache.org/beam-fn-api-control-data-embedding.
Review comment:
```suggestion
// (optional) Output elements of the processed bundle. Either all or
// none of the bundle elements should be included in the ProcessBundleResponse.
// This embedding is to achieve better efficiency for bundles
// that contain only small amounts of data. See more at
// https://s.apache.org/beam-fn-api-control-data-embedding.
```
##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -296,6 +296,14 @@ message ProcessBundleRequest {
// (Optional) A list of cache tokens that can be used by an SDK to reuse
// cached data returned by the State API across multiple bundles.
repeated CacheToken cache_tokens = 2;
+
+ // (optional) Elements to be processed with the bundle. Either all or
+ // none of the bundle elements should be included in the ProcessBundleRequest.
+ // This elements embedding is to achieve better Fn API efficiency for bundles
+ // that contain only small amount of data and are cheap to be processed on the
+ // SDK harness side. See more at
+ // https://s.apache.org/beam-fn-api-control-data-embedding.
+ Elements elements = 3;
Review comment:
Please add to the comment something like: `This field will can be set if the SDK declares that they support the "...." capability.`
##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -296,6 +296,14 @@ message ProcessBundleRequest {
// (Optional) A list of cache tokens that can be used by an SDK to reuse
// cached data returned by the State API across multiple bundles.
repeated CacheToken cache_tokens = 2;
+
+ // (optional) Elements to be processed with the bundle. Either all or
+ // none of the bundle elements should be included in the ProcessBundleRequest.
+ // This elements embedding is to achieve better Fn API efficiency for bundles
+ // that contain only small amount of data and are cheap to be processed on the
+ // SDK harness side. See more at
+ // https://s.apache.org/beam-fn-api-control-data-embedding.
+ Elements elements = 3;
Review comment:
```suggestion
// (optional) Elements to be processed within the bundle. Either all or
// none of the elements should be included in the ProcessBundleRequest.
// This embedding is to achieve better efficiency for bundles
// that contain only small amounts of data. See more at
// https://s.apache.org/beam-fn-api-control-data-embedding.
Elements elements = 3;
```
##########
File path: model/pipeline/src/main/proto/beam_runner_api.proto
##########
@@ -1579,6 +1585,12 @@ message StandardRunnerProtocols {
enum Enum {
// Indicates suport the MonitoringInfo short id protocol.
MONITORING_INFO_SHORT_IDS = 0 [(beam_urn) = "beam:protocol:monitoring_info_short_ids:v1"];
+
+ // Indicates that this runner can process elements embedded in Fn API
+ // control plane ProcessBundleResponse. See more about the protocol at
+ // https://s.apache.org/beam-fn-api-control-data-embedding
Review comment:
```suggestion
// Indicates that this runner can process elements embedded in the
// ProcessBundleResponse. See more about the protocol at
// https://s.apache.org/beam-fn-api-control-data-embedding
```
##########
File path: model/pipeline/src/main/proto/beam_runner_api.proto
##########
@@ -1570,6 +1570,12 @@ message StandardProtocols {
// the entire SDK harness process, not specific to a bundle.
HARNESS_MONITORING_INFOS = 4
[(beam_urn) = "beam:protocol:harness_monitoring_infos:v1"];
+
+ // Indicates that this SDK can process elements embedded in Fn API
+ // control plane ProcessBundleRequest. See more about the protocol at
+ // https://s.apache.org/beam-fn-api-control-data-embedding
Review comment:
```suggestion
// Indicates that this SDK can process elements embedded in the
// ProcessBundleRequest. See more about the protocol at
// https://s.apache.org/beam-fn-api-control-data-embedding
```
##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -327,6 +335,13 @@ message ProcessBundleResponse {
// payload field with the bytes specified here.
map<string, bytes> monitoring_data = 5;
+ // (optional) Output elements of the processed bundle. Either all or
+ // none of the bundle elements should be included in the ProcessBundleResponse.
+ // This elements embedding is to achieve better Fn API efficiency for bundles
+ // that only produce small amount of outputs. See more at
+ // https://s.apache.org/beam-fn-api-control-data-embedding.
Review comment:
Please add another comment stating that: `This field can only be set if the runner declares that they support the "...." capability.`
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org