justinpakzad commented on PR #63575: URL: https://github.com/apache/airflow/pull/63575#issuecomment-4061085999
> Thanks for your PR! While reviewing this, I thought of a couple of edge cases regarding the `timeout `parameter: > > 1. User set timeout over 604800 > 2. User set timeout to a negative number > > Should we add some validation logic to raise a `ValueError `for negative numbers and either cap the value at 604800 (with a warning) or raise an error if it exceeds the max limit? This would help us fail fast and provide a better error message to the users. Hey Henry, thanks for the review. Good callout. This was something I did consider but I didn't see a lot of client side validation happening in the SF hooks. For example, if we look at `statement_count` in the same method - there is no validation to check if that's negative or within a certain range, that would be handled by Snowflake. I just ran some tests against my SF account and it appears that negative and values > 604800 are both valid and do not actually throw errors. For negative values it would just stop executing immediately and for the larger ones I think it probably defaults to 604800 (I couldn't check for certain). I would be curious to hear what one of the maintainers has to say on whether or not Snowflake should be handling this kind of stuff or we should be adding these checks. If that's the case I'm happy to make that update - but then we should probably do the same for statement counts and any other similar cases. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
