fix(dataproc): add mising fields in yarnApplication proto#16868
fix(dataproc): add mising fields in yarnApplication proto#16868blcksrx wants to merge 1 commit intogoogleapis:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds vcore_seconds and memory_mb_seconds fields to the YarnApplication message in the Dataproc V1 library. Feedback was provided to remove the optional=True parameter from these fields to ensure consistency with the backend proto definition and existing fields. Additionally, the docstrings should be updated to remove the 'Optional.' prefix and use more descriptive unit names like 'megabyte-seconds' for clarity.
| vcore_seconds: int = proto.Field( | ||
| proto.INT64, | ||
| number=5, | ||
| optional=True, | ||
| ) | ||
| memory_mb_seconds: int = proto.Field( | ||
| proto.INT64, | ||
| number=6, | ||
| optional=True, | ||
| ) |
There was a problem hiding this comment.
The optional=True parameter should be removed. According to the official Dataproc V1 YarnApplication proto definition, these fields are standard int64 fields and do not use the optional keyword. Adding optional=True in proto-plus enables explicit presence tracking, which is inconsistent with the existing fields in this message (like field 4) and the backend proto definition.
vcore_seconds: int = proto.Field(
proto.INT64,
number=5,
)
memory_mb_seconds: int = proto.Field(
proto.INT64,
number=6,
)| vcore_seconds (int): | ||
| Optional. The cumulative CPU time consumed by the application for a job, measured in vcore-seconds. | ||
| memory_mb_seconds (int): | ||
| Optional. The cumulative memory usage of the application for a job, measured in mb-seconds. |
There was a problem hiding this comment.
The docstring for these fields should be updated for consistency and accuracy. Specifically:
- The "Optional." prefix is not standard for message fields in this library and should be removed, as these fields are not marked as optional in the official Dataproc proto.
- The unit for
memory_mb_secondsshould be "megabyte-seconds" to match the official documentation and provide better clarity than "mb-seconds". - Note that the
progressfield (field 4) appears to be missing from the docstring betweentracking_urlandvcore_seconds.
| vcore_seconds (int): | |
| Optional. The cumulative CPU time consumed by the application for a job, measured in vcore-seconds. | |
| memory_mb_seconds (int): | |
| Optional. The cumulative memory usage of the application for a job, measured in mb-seconds. | |
| vcore_seconds (int): | |
| The cumulative CPU time consumed by the application for a job, measured in vcore-seconds. | |
| memory_mb_seconds (int): | |
| The cumulative memory usage of the application for a job, measured in megabyte-seconds. |
8fe9677 to
f95894a
Compare
|
Regrettably, this PR can't be merged. The code is automatically generated from the |
|
@parthea Thank you very much for your answer, is it possible to contribute to the mentioned project to speed up the process deilivery? |
|
@blcksrx, Regrettably, this change needs to be made in an internal code base by the team that maintains this product. You can reach them directly using the email address here. If you've filed a Google Cloud Support issue for this, your support contact can also reach the team via internal issue b/507808022. |
Two fields in
YarnApplicationProtois missing:vcore_scondsmemory_mb_secondsThis Pr, is a fix for missing these fields.
Reference:
https://docs.cloud.google.com/dataproc/docs/reference/rpc/google.cloud.dataproc.v1#google.cloud.dataproc.v1.YarnApplication
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕