Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-35232] Add retry settings for GCS connector #24753

Closed
wants to merge 2 commits into from

Conversation

JTaky
Copy link
Contributor

@JTaky JTaky commented May 1, 2024

Provide a way to ingest retrier settings configuration to gcs-cloud-storage library in use.

Brief change log

  • exposed gs.retry.max-attempt, gs.retry.init-rpc-timeout, gs.retry.rpc-timeout-multiplier, and gs.retry.rpc-timeout-multiplier and gs.retry.total-timeout configuration.
  • Pluming retry settings configurations to gcs-cloud-storage client
  • update the documentation for GCS

Verifying this change

  • This change is already covered by existing tests, such as GSRecoverableWriterTest*.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): no
  • The serializers: no
  • The runtime per-record code paths (performance sensitive): no
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
  • The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? yes
If yes, how is the feature documented? docs

@flinkbot
Copy link
Collaborator

flinkbot commented May 1, 2024

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@JTaky JTaky force-pushed the oleksandr.nitavskyi/gcs_timeout branch from 1fb740a to 07f00e1 Compare May 3, 2024 13:23
Copy link
Contributor

@xintongsong xintongsong 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 opening this PR, @JTaky. It looks good to me in general. I have only one minor comment. Please take a look.

public static final ConfigOption<Integer> GCS_RETRY_MAX_ATTEMPT =
ConfigOptions.key("gs.retry.max-attempt")
.intType()
.defaultValue(6)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about setting default values for these options. The default values are ignored anyway because these options are only used in getOptional(), which returns empty() if the option is not explicitly configured by user. We can mention in the description that if not configured, the GCS default values will be used.

@JTaky JTaky force-pushed the oleksandr.nitavskyi/gcs_timeout branch from e9266ef to 617b14b Compare May 8, 2024 12:31
@JTaky JTaky requested a review from xintongsong May 8, 2024 20:30
Copy link
Contributor

@xintongsong xintongsong 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 addressing my comments. LGTM. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants