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

#52: Added CLI options for specifying the timeout for the ExtractValidator #57

Merged
merged 14 commits into from
Oct 14, 2024

Conversation

ckunki
Copy link
Contributor

@ckunki ckunki commented Aug 20, 2024

Closes #52

and changed extract-timeout to be specified in minutes with default 5 minutes.
@@ -306,7 +310,10 @@ def create(cls,
path_in_bucket: str = '',
use_ssl_cert_validation: bool = True, ssl_trusted_ca: Optional[str] = None,
ssl_client_certificate: Optional[str] = None,
ssl_private_key: Optional[str] = None) -> "LanguageContainerDeployer":
ssl_private_key: Optional[str] = None,
extract_timeout: timedelta = timedelta(seconds=10),
Copy link
Collaborator

Choose a reason for hiding this comment

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

A proper value for a timeout is beyond 10 minutes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will set it to 10 minutes, see next push.

@@ -152,6 +153,9 @@ def secret_callback(ctx: click.Context, param: click.Option, value: Any):
@click.option('--alter-system/--no-alter-system', type=bool, default=True)
@click.option('--allow-override/--disallow-override', type=bool, default=False)
@click.option('--wait_for_completion/--no-wait_for_completion', type=bool, default=True)
@click.option('--extract-timeout-minutes', type=int, default=5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would set the default at least to 10 minutes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See next push.

@@ -87,6 +87,10 @@ def get_udf_path(bucket_base_path: bfs.path.PathLike, bucket_file: str) -> PureP
return PurePosixPath(file_path.as_udf_path())


def display_extract_progress(n: int, pending: List[int]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

How often would this be called?

Copy link
Contributor Author

@ckunki ckunki Sep 23, 2024

Choose a reason for hiding this comment

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

It depends on the settings.

  • timeout by default is now updated to 10 minutes, can be changed via CLI
  • interval currently, is always set to 10 seconds, not configurable via CLI

So with these setting, by default the callback will be called 10*60/10 = 60 times at max.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I probably, would increase the interval to 30 seconds

Copy link
Contributor Author

@ckunki ckunki Oct 10, 2024

Choose a reason for hiding this comment

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

Thanks - changed in latest push.

@ahsimb
Copy link
Collaborator

ahsimb commented Oct 9, 2024

I am not sure if the term extract would make sense to the user. This is a kind of knowledge bias on our side. Could deploy-timeout-minutes be a better name?

@tkilias
Copy link
Collaborator

tkilias commented Oct 10, 2024

I am not sure if the term extract would make sense to the user. This is a kind of knowledge bias on our side. Could deploy-timeout-minutes be a better name?

Yes, I think that is a good idea

tkilias
tkilias previously approved these changes Oct 10, 2024
@ckunki ckunki merged commit 91c737c into main Oct 14, 2024
10 checks passed
@ckunki ckunki deleted the feature/#52-Added_timeout_options_for_SLC_deployer_to_CLI branch October 14, 2024 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SLC Deployer: Add timeout option to CLI
3 participants