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 2020/10/22 22:01:38 UTC
[GitHub] [beam] lukecwik commented on a change in pull request #13163: [BEAM-11092] Add protos for new process wide HarnessMonitoringInfos, and BigQuery IO metrics
lukecwik commented on a change in pull request #13163:
URL: https://github.com/apache/beam/pull/13163#discussion_r510480672
##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -135,12 +137,55 @@ message InstructionResponse {
ProcessBundleSplitResponse process_bundle_split = 1003;
FinalizeBundleResponse finalize_bundle = 1004;
MonitoringInfosMetadataResponse monitoring_infos = 1005;
+ HarnessMonitoringInfosResponse harness_monitoring_infos = 1006;
+ HarnessMonitoringInfosMetadataResponse harness_monitoring_infos_metadata = 1007;
// DEPRECATED
RegisterResponse register = 1000;
}
}
+// A request to provide full MonitoringInfo associated with the entire SDK
+// harness process, not specific to a bundle.
+//
+// An SDK can report metrics using an identifier that only contains the
+// associated payload. A runner who wants to receive the full metrics
+// information can request all the monitoring metadata via a
+// MonitoringInfosMetadataRequest providing a list of ids as necessary.
+//
+// The SDK is allowed to reuse the identifiers across multiple bundles as long
+// as the MonitoringInfo could be reconstructed fully by overwriting its
+// payload field with the bytes specified here.
Review comment:
```suggestion
// The SDK is allowed to reuse the identifiers for the lifetime of the associated control connection as long
// as the MonitoringInfo could be reconstructed fully by overwriting its
// payload field with the bytes specified here.
```
##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -108,6 +108,8 @@ message InstructionRequest {
ProcessBundleSplitRequest process_bundle_split = 1003;
FinalizeBundleRequest finalize_bundle = 1004;
MonitoringInfosMetadataRequest monitoring_infos = 1005;
+ HarnessMonitoringInfosRequest harness_monitoring_infos = 1006;
+ HarnessMonitoringInfosMetadataRequest harness_monitoring_infos_metadata = 1007;
Review comment:
I thought that this would be covered by MonitoringInfosMetadataRequest
```suggestion
```
##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -135,12 +137,55 @@ message InstructionResponse {
ProcessBundleSplitResponse process_bundle_split = 1003;
FinalizeBundleResponse finalize_bundle = 1004;
MonitoringInfosMetadataResponse monitoring_infos = 1005;
+ HarnessMonitoringInfosResponse harness_monitoring_infos = 1006;
+ HarnessMonitoringInfosMetadataResponse harness_monitoring_infos_metadata = 1007;
// DEPRECATED
RegisterResponse register = 1000;
}
}
+// A request to provide full MonitoringInfo associated with the entire SDK
+// harness process, not specific to a bundle.
+//
+// An SDK can report metrics using an identifier that only contains the
+// associated payload. A runner who wants to receive the full metrics
+// information can request all the monitoring metadata via a
+// MonitoringInfosMetadataRequest providing a list of ids as necessary.
+//
+// The SDK is allowed to reuse the identifiers across multiple bundles as long
+// as the MonitoringInfo could be reconstructed fully by overwriting its
+// payload field with the bytes specified here.
+message HarnessMonitoringInfosRequest {
+}
+
+message HarnessMonitoringInfosResponse {
+ // An identifier to MonitoringInfo.payload mapping containing
+ // Metrics associated with the sdk harness process, not a specific bundle.
+ //
+ // An SDK can report metrics using an identifier that only contains the
+ // associated payload. A runner who wants to receive the full metrics
+ // information can request all the monitoring metadata via a
+ // MonitoringInfosMetadataRequest providing a list of ids as necessary.
+ //
+ // The SDK is allowed to reuse the identifiers across multiple bundles as long
Review comment:
```suggestion
// The SDK is allowed to reuse the identifiers for the lifetime of the associated control connection as long
```
##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -306,18 +351,18 @@ message ProcessBundleProgressRequest {
string instruction_id = 1;
}
-// A request to provide full MonitoringInfo for a given id.
+// A request to provide full MonitoringInfo for a given bundle.
Review comment:
```suggestion
// A request to provide full MonitoringInfo for a set of provided ids.
```
##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -135,12 +137,55 @@ message InstructionResponse {
ProcessBundleSplitResponse process_bundle_split = 1003;
FinalizeBundleResponse finalize_bundle = 1004;
MonitoringInfosMetadataResponse monitoring_infos = 1005;
+ HarnessMonitoringInfosResponse harness_monitoring_infos = 1006;
+ HarnessMonitoringInfosMetadataResponse harness_monitoring_infos_metadata = 1007;
Review comment:
```suggestion
```
##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -349,11 +394,11 @@ message ProcessBundleProgressResponse {
// information can request all the monitoring metadata via a
// MonitoringInfosMetadataRequest providing a list of ids as necessary.
//
-// The MonitoringInfo ids are scoped to the associated control connection. For
-// example an SDK may reuse the ids across multiple bundles.
+// The SDK is allowed to reuse the identifiers across multiple bundles as long
Review comment:
```suggestion
// The SDK is allowed to reuse the identifiers for the lifetime of the associated control connection as long
```
##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -135,12 +137,55 @@ message InstructionResponse {
ProcessBundleSplitResponse process_bundle_split = 1003;
FinalizeBundleResponse finalize_bundle = 1004;
MonitoringInfosMetadataResponse monitoring_infos = 1005;
+ HarnessMonitoringInfosResponse harness_monitoring_infos = 1006;
+ HarnessMonitoringInfosMetadataResponse harness_monitoring_infos_metadata = 1007;
// DEPRECATED
RegisterResponse register = 1000;
}
}
+// A request to provide full MonitoringInfo associated with the entire SDK
+// harness process, not specific to a bundle.
+//
+// An SDK can report metrics using an identifier that only contains the
+// associated payload. A runner who wants to receive the full metrics
+// information can request all the monitoring metadata via a
+// MonitoringInfosMetadataRequest providing a list of ids as necessary.
+//
+// The SDK is allowed to reuse the identifiers across multiple bundles as long
+// as the MonitoringInfo could be reconstructed fully by overwriting its
+// payload field with the bytes specified here.
+message HarnessMonitoringInfosRequest {
+}
+
+message HarnessMonitoringInfosResponse {
+ // An identifier to MonitoringInfo.payload mapping containing
+ // Metrics associated with the sdk harness process, not a specific bundle.
Review comment:
```suggestion
// Metrics associated with the SDK harness, not a specific bundle.
```
##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -306,18 +351,18 @@ message ProcessBundleProgressRequest {
string instruction_id = 1;
}
-// A request to provide full MonitoringInfo for a given id.
+// A request to provide full MonitoringInfo for a given bundle.
//
// An SDK can report metrics using an identifier that only contains the
// associated payload. A runner who wants to receive the full metrics
// information can request all the monitoring metadata via a
// MonitoringInfosMetadataRequest providing a list of ids as necessary.
//
-// The MonitoringInfo ids are scoped to the associated control connection. For
-// example, an SDK may reuse the ids across multiple bundles.
+// The SDK is allowed to reuse the identifiers across multiple bundles as long
+// as the MonitoringInfo could be reconstructed fully by overwriting its
+// payload field with the bytes specified here.
message MonitoringInfosMetadataRequest {
- // A list of ids for which MonitoringInfo are requested. All but the payload
- // field will be populated.
+ // A list of ids for which the full MonitoringInfo is requested for.
repeated string monitoring_info_id = 1;
}
Review comment:
Can you copy the update to say `for the lifetime of the associated control connection` in ProcessBundleProgressResponse and ProcessBundleResponse
##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -306,18 +351,18 @@ message ProcessBundleProgressRequest {
string instruction_id = 1;
}
-// A request to provide full MonitoringInfo for a given id.
+// A request to provide full MonitoringInfo for a given bundle.
//
// An SDK can report metrics using an identifier that only contains the
// associated payload. A runner who wants to receive the full metrics
// information can request all the monitoring metadata via a
// MonitoringInfosMetadataRequest providing a list of ids as necessary.
//
-// The MonitoringInfo ids are scoped to the associated control connection. For
-// example, an SDK may reuse the ids across multiple bundles.
+// The SDK is allowed to reuse the identifiers across multiple bundles as long
Review comment:
```suggestion
// The SDK is allowed to reuse the identifiers for the lifetime of the associated control connection as long
```
##########
File path: model/fn-execution/src/main/proto/beam_fn_api.proto
##########
@@ -135,12 +137,55 @@ message InstructionResponse {
ProcessBundleSplitResponse process_bundle_split = 1003;
FinalizeBundleResponse finalize_bundle = 1004;
MonitoringInfosMetadataResponse monitoring_infos = 1005;
+ HarnessMonitoringInfosResponse harness_monitoring_infos = 1006;
+ HarnessMonitoringInfosMetadataResponse harness_monitoring_infos_metadata = 1007;
// DEPRECATED
RegisterResponse register = 1000;
}
}
+// A request to provide full MonitoringInfo associated with the entire SDK
+// harness process, not specific to a bundle.
+//
+// An SDK can report metrics using an identifier that only contains the
+// associated payload. A runner who wants to receive the full metrics
+// information can request all the monitoring metadata via a
+// MonitoringInfosMetadataRequest providing a list of ids as necessary.
+//
+// The SDK is allowed to reuse the identifiers across multiple bundles as long
+// as the MonitoringInfo could be reconstructed fully by overwriting its
+// payload field with the bytes specified here.
+message HarnessMonitoringInfosRequest {
+}
+
+message HarnessMonitoringInfosResponse {
+ // An identifier to MonitoringInfo.payload mapping containing
+ // Metrics associated with the sdk harness process, not a specific bundle.
+ //
+ // An SDK can report metrics using an identifier that only contains the
+ // associated payload. A runner who wants to receive the full metrics
+ // information can request all the monitoring metadata via a
+ // MonitoringInfosMetadataRequest providing a list of ids as necessary.
+ //
+ // The SDK is allowed to reuse the identifiers across multiple bundles as long
+ // as the MonitoringInfo could be reconstructed fully by overwriting its
+ // payload field with the bytes specified here.
+ map<string, bytes> monitoring_data = 1;
+}
+
+message HarnessMonitoringInfosMetadataRequest {
+ // A list of ids for which the full MonitoringInfo is requested for.
+ repeated string monitoring_info_id = 1;
+}
+
+message HarnessMonitoringInfosMetadataResponse {
+ // A mapping from an identifier to the full metrics information.
+ // The “bytes payload” field is not populated on the MonitoringInfo
+ map<string, org.apache.beam.model.pipeline.v1.MonitoringInfo>
+ monitoring_infos = 1;
+}
+
Review comment:
```suggestion
```
----------------------------------------------------------------
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