vincbeck commented on code in PR #22737:
URL: https://github.com/apache/airflow/pull/22737#discussion_r847437905


##########
airflow/providers/amazon/aws/sensors/s3.py:
##########
@@ -109,39 +152,18 @@ def get_hook(self) -> S3Hook:
         return self.hook
 
 
-class S3KeySizeSensor(S3KeySensor):
-    """
-    Waits for a key (a file-like instance on S3) to be present and be more than
-    some size in a S3 bucket.
-    S3 being a key/value it does not support folders. The path is just a key
-    a resource.
+def default_check_fn(data: List) -> bool:

Review Comment:
   > Why is this function not in the S3KeySizeSensor class?
   
   Yeah, after thinking it about it, it makes more sense. Let me put it back
   
   > Can you please explain this change and how is it backward compatible?
   
   `S3KeySensor` waits a file to exist in a S3 bucket. `S3KeySizeSensor` waits 
a file to exist in a S3 bucket + add a custom check to check the size (which 
actually should not be limited to the size, hence the name is already wrong). 
But anyway `S3KeySizeSensor` does exactly what `S3KeySensor` does + it adds an 
additional custom check. Let's just add that custom check as optional parameter.
   
   > I see you created the howto_sensor_s3_key_function_definition example but 
reading the doc you just referenced it I think now that S3KeySensor has several 
different capabilities we should list them in the doc and explain about them. 
It wasn't so clear to me when reading it
   
   I actually thought that's exactly what I did :) I listed multiple use cases 
in the doc:
   - To check one file:
   - To check multiple files:
   - To check with an additional check
   
   Please tell me what is not clear or how I can improve it



-- 
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]

Reply via email to