Support serialization and deserialization for GoogleCloudStorageReadOptions#39445
Support serialization and deserialization for GoogleCloudStorageReadOptions#39445shunping wants to merge 5 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
r: @Abacn |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
There was a problem hiding this comment.
The size of this fix is non-trivial. There are a few concerns
-
reflection based, generally fragile
-
It also increases serialized size of pipeline options (seen in Dataflow UI, internal objects, etc).
Given that get/setGoogleCloudStorageReadOptions are deprecated and is only effective in GcsUtilV1, I would prefer a smaller fix:
-
Only json-serialize-deserialize options that we want to explicitly expose (Fadvise, etc). On serialization, when it's found not the default, write to encoding. In this way we can avoid reflection and largely reduce the serialization size
-
If possible, do not leak jackson classes to ApiSurface
There was a problem hiding this comment.
If our goal is solely to fix the regression caused by the default value of fadvise, we wouldn't need to modify the serialization and deserialization logic for GoogleCloudStorageReadOptions.
However, the broader intent of this fix is to address the underlying issue: the inability to propagate GoogleCloudStorageReadOptions to remote workers via pipeline options.
Regarding the use of reflection, it was chosen to future-proof the implementation. If we explicitly serialize fields instead of using reflection, we will be forced to update our code every time the upstream gcs-connector modifies or adds fields to this class.
There was a problem hiding this comment.
I made the change to support a subset of options.
However, Jackson classes must be included in the ApiSurface. This is because GcsOptions is a public interface with methods annotated with @JsonSerialize and @JsonDeserialize. The JsonDeserialize annotation declares methods returning Class<? extends JsonDeserializer>, which pulls com.fasterxml.jackson.databind into the exposed API surface graph. JsonDeserializer, in turn, references JsonParser and other classes from com.fasterxml.jackson.core.
…ptions Fixes an issue where GoogleCloudStorageReadOptions set on GcsOptions was annotated with @JsonIgnore, causing it to be omitted when serializing PipelineOptions for remote workers (e.g., Dataflow) and falling back to defaults.
This fixes the regression introduced by gcs-connector v3.
efc6f35 to
74d2358
Compare
This PR fixes a bug where annotating
GoogleCloudStorageReadOptionswith@JsonIgnorecaused custom GCS read options (such asfadvise) to be dropped during JSON serialization, forcing remote workers to fall back to default settings.Additionally, in
gcs-connectorv3,GoogleCloudStorageReadOptions.DEFAULTchanged its defaultfadvisefromSEQUENTIALtoAUTO.Beam workloads default to
SEQUENTIALinGcsReadOptionsFactoryto preserve expected sequential read throughput and caching behavior.Going forward, users can configure
GoogleCloudStorageReadOptionsso that they successfully take effect on workers using a code snippet like the following: