[GitHub] [airflow] uranusjr commented on a diff in pull request #29245: fix code checking job names in sagemaker

["uranusjr (via GitHub)" <gi...@apache.org>]

uranusjr commented on code in PR #29245:
URL: https://github.com/apache/airflow/pull/29245#discussion_r1113909968


##########
airflow/providers/amazon/aws/operators/sagemaker.py:
##########
@@ -106,6 +108,41 @@ def _create_integer_fields(self) -> None:
         """
         self.integer_fields = []
 
+    def _get_unique_job_name(
+        self, proposed_name: str, fail_if_exists: bool, describe_func: Callable[[str], Any]
+    ) -> str:
+        """
+        Returns the proposed name if it doesn't already exist, otherwise returns it with a random suffix.
+
+        :param proposed_name: Base name.
+        :param fail_if_exists: Will throw an error if a job with that name already exists
+            instead of finding a new name.
+        :param describe_func: The `describe_` function for that kind of job.
+            We use it as an O(1) way to check if a job exists.
+        """
+        job_name = proposed_name
+        while self._check_if_job_exists(job_name, describe_func):
+            # this while should loop only once in most cases, just setting it this way to regenerate a name
+            # in case there is a random number collision.
+            if fail_if_exists:
+                raise AirflowException(f"A SageMaker job with name {job_name} already exists.")
+            else:
+                job_name = f"{proposed_name}-{random.randint(0, 999999999):09}"

Review Comment:
   Regarding reproduction, would it be a good idea to use a time-based token, say `time.time()`, here? It would still be somewhat “random” but more predictable.



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org