You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "nsAstro (via GitHub)" <gi...@apache.org> on 2023/02/23 21:00:24 UTC
[GitHub] [airflow] nsAstro opened a new pull request, #29732: Loop through paginated response to check for cluster id
nsAstro opened a new pull request, #29732:
URL: https://github.com/apache/airflow/pull/29732
Loops through the paginated emr list_clusters endpoint and searches each set for the given cluster_id.
closes: #29712
--
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
[GitHub] [airflow] nsAstro commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "nsAstro (via GitHub)" <gi...@apache.org>.
nsAstro commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1125242250
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -191,6 +197,9 @@ def test_get_cluster_id_by_name(self):
assert no_match is None
+ with pytest.raises(AirflowException):
+ hook.get_cluster_id_by_name("test_cluster0", ["RUNNING", "WAITING", "BOOTSTRAPPING"])
+
Review Comment:
Their API [docs](https://docs.aws.amazon.com/emr/latest/APIReference/API_ListClusters.html) says unordered list. I could change the internal logic of the function to order the cluster, but that feels silly to do just to make a test.
--
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
[GitHub] [airflow] nsAstro commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "nsAstro (via GitHub)" <gi...@apache.org>.
nsAstro commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1125238948
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -191,6 +197,9 @@ def test_get_cluster_id_by_name(self):
assert no_match is None
+ with pytest.raises(AirflowException):
+ hook.get_cluster_id_by_name("test_cluster0", ["RUNNING", "WAITING", "BOOTSTRAPPING"])
+
Review Comment:
I'm not sure if list-clusters API on EMR is ordered at all, so I don't know if there's a way to guarantee that a given test-case cluster shows up on the second page only. If there's a way to guarantee this, I can add that as a test. I'll poke through the AWS docs to see if they provide some sort of ordering on how they list clusters to see if there's a deterministic test case I can write.
--
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
[GitHub] [airflow] nsAstro commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "nsAstro (via GitHub)" <gi...@apache.org>.
nsAstro commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1126582422
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -191,6 +197,9 @@ def test_get_cluster_id_by_name(self):
assert no_match is None
+ with pytest.raises(AirflowException):
+ hook.get_cluster_id_by_name("test_cluster0", ["RUNNING", "WAITING", "BOOTSTRAPPING"])
+
Review Comment:
I totally understand, I just am not sure how to force a given clusterName to be on the second page. Can I assume that even if the listClusters API provides an unordered response, it will provide that same order in subsequent queries? Then I can do a regular listClusters and fetch a cluster on the second page. But if the ordering is not consistent, I don't really have a surefire way to guarantee that the clusterName is on the second page. I could increase the number of clusters (<1000) to make the probability very high.
I'm happy to add tests, even for testing duplicate clusterNames which wasn't part of this PR. I'm just not too familiar with the EMR API, so I'm looking for some guidance.
--
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
[GitHub] [airflow] o-nikolas commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1179752000
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -191,6 +201,24 @@ def test_get_cluster_id_by_name(self):
assert no_match is None
+ with pytest.raises(AirflowException):
Review Comment:
Addressed
--
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
[GitHub] [airflow] o-nikolas commented on pull request #29732: Loop through paginated response to check for cluster id
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #29732:
URL: https://github.com/apache/airflow/pull/29732#issuecomment-1526405232
It doesn't look like @nsAstro is enganged on this one any more. I'll push the remaining fixes if I get some time in the next few days.
--
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
[GitHub] [airflow] o-nikolas commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1125153517
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -181,6 +182,11 @@ def test_get_cluster_id_by_name(self):
{"Name": "test_cluster", "Instances": {"KeepJobFlowAliveWhenNoSteps": True}}
)
+ for i in range(51):
+ hook.create_job_flow(
+ {"Name": "test_cluster" + str(i % 50), "Instances": {"KeepJobFlowAliveWhenNoSteps": True}}
Review Comment:
Did you use `i % 50` so that the 0 case would be in the list twice? If so it's a bit sneaky and non-obvious IMHO.
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -191,6 +197,9 @@ def test_get_cluster_id_by_name(self):
assert no_match is None
+ with pytest.raises(AirflowException):
+ hook.get_cluster_id_by_name("test_cluster0", ["RUNNING", "WAITING", "BOOTSTRAPPING"])
+
Review Comment:
Should you not add a test case which covers the issue this PR is solving? I.e. A success case for getting a cluster id for a cluster that would be on the second page.
--
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
[GitHub] [airflow] nsAstro commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "nsAstro (via GitHub)" <gi...@apache.org>.
nsAstro commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1125237240
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -181,6 +182,11 @@ def test_get_cluster_id_by_name(self):
{"Name": "test_cluster", "Instances": {"KeepJobFlowAliveWhenNoSteps": True}}
)
+ for i in range(51):
+ hook.create_job_flow(
+ {"Name": "test_cluster" + str(i % 50), "Instances": {"KeepJobFlowAliveWhenNoSteps": True}}
Review Comment:
Yes I did. Was just a fast way to test, but I can add a comment to make it clearer or just hardcode a more explicit duplicated cluster name.
--
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
[GitHub] [airflow] o-nikolas commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1128757681
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -191,6 +201,24 @@ def test_get_cluster_id_by_name(self):
assert no_match is None
+ with pytest.raises(AirflowException):
Review Comment:
+1
--
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
[GitHub] [airflow] nsAstro commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "nsAstro (via GitHub)" <gi...@apache.org>.
nsAstro commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1125238948
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -191,6 +197,9 @@ def test_get_cluster_id_by_name(self):
assert no_match is None
+ with pytest.raises(AirflowException):
+ hook.get_cluster_id_by_name("test_cluster0", ["RUNNING", "WAITING", "BOOTSTRAPPING"])
+
Review Comment:
I'm not sure if list-clusters API on EMR is ordered at all, so I don't know if there's a way to guarantee that a given test-case cluster shows up on the second page only. If there's a way to guarantee this, I can add that as a test.
--
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
[GitHub] [airflow] nsAstro commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "nsAstro (via GitHub)" <gi...@apache.org>.
nsAstro commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1125237240
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -181,6 +182,11 @@ def test_get_cluster_id_by_name(self):
{"Name": "test_cluster", "Instances": {"KeepJobFlowAliveWhenNoSteps": True}}
)
+ for i in range(51):
+ hook.create_job_flow(
+ {"Name": "test_cluster" + str(i % 50), "Instances": {"KeepJobFlowAliveWhenNoSteps": True}}
Review Comment:
Yes I did. Was just a fast way to test, but I can add a comment to make it clearer or just hardcode a loop around a different string.
--
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
[GitHub] [airflow] vincbeck commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1128634322
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -191,6 +201,24 @@ def test_get_cluster_id_by_name(self):
assert no_match is None
+ with pytest.raises(AirflowException):
Review Comment:
LGTM but I would move it to a separate test for clarity purposes. The shorter the tests are, the more likely people will read them, the better :)
--
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
[GitHub] [airflow] Taragolis commented on pull request #29732: Loop through paginated response to check for cluster id
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29732:
URL: https://github.com/apache/airflow/pull/29732#issuecomment-1442456791
> I took a stab at this but I am at a loss as to how to test without spinning up 51 EMR clusters myself. If anyone has a suggestion for how to whitebox, mock, or test this, happy to contribute.
You could mock this method and use [side_effect](https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock)
--
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
[GitHub] [airflow] o-nikolas commented on pull request #29732: Loop through paginated response to check for cluster id
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #29732:
URL: https://github.com/apache/airflow/pull/29732#issuecomment-1526640612
I updated the PR to address the remaining feedback
--
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
[GitHub] [airflow] nsAstro commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "nsAstro (via GitHub)" <gi...@apache.org>.
nsAstro commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1126610895
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -191,6 +197,9 @@ def test_get_cluster_id_by_name(self):
assert no_match is None
+ with pytest.raises(AirflowException):
+ hook.get_cluster_id_by_name("test_cluster0", ["RUNNING", "WAITING", "BOOTSTRAPPING"])
+
Review Comment:
I just realized with the way the marker works, that listClusters will be consistent. I'll edit the test accordingly.
--
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
[GitHub] [airflow] thirtyseven commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "thirtyseven (via GitHub)" <gi...@apache.org>.
thirtyseven commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1116320813
##########
airflow/providers/amazon/aws/hooks/emr.py:
##########
@@ -68,22 +68,24 @@ def get_cluster_id_by_name(self, emr_cluster_name: str, cluster_states: list[str
:param emr_cluster_name: Name of a cluster to find
:param cluster_states: State(s) of cluster to find
:return: id of the EMR cluster
- """
+ """
response = self.get_conn().list_clusters(ClusterStates=cluster_states)
+ while true:
+ matching_clusters = list(
+ filter(lambda cluster: cluster["Name"] == emr_cluster_name, response["Clusters"])
+ )
- matching_clusters = list(
- filter(lambda cluster: cluster["Name"] == emr_cluster_name, response["Clusters"])
- )
-
- if len(matching_clusters) == 1:
- cluster_id = matching_clusters[0]["Id"]
- self.log.info("Found cluster name = %s id = %s", emr_cluster_name, cluster_id)
- return cluster_id
- elif len(matching_clusters) > 1:
- raise AirflowException(f"More than one cluster found for name {emr_cluster_name}")
- else:
- self.log.info("No cluster found for name %s", emr_cluster_name)
- return None
+ if len(matching_clusters) == 1:
+ cluster_id = matching_clusters[0]["Id"]
+ self.log.info("Found cluster name = %s id = %s", emr_cluster_name, cluster_id)
+ return cluster_id
+ elif len(matching_clusters) > 1:
+ raise AirflowException(f"More than one cluster found for name {emr_cluster_name}")
+ elif response["Marker"]:
Review Comment:
boto has a builtin pagination API, it would be nicer to use it rather than reimplement the logic:
https://boto3.amazonaws.com/v1/documentation/api/latest/guide/paginators.html
--
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
[GitHub] [airflow] thirtyseven commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "thirtyseven (via GitHub)" <gi...@apache.org>.
thirtyseven commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1116320465
##########
airflow/providers/amazon/aws/hooks/emr.py:
##########
@@ -68,22 +68,24 @@ def get_cluster_id_by_name(self, emr_cluster_name: str, cluster_states: list[str
:param emr_cluster_name: Name of a cluster to find
:param cluster_states: State(s) of cluster to find
:return: id of the EMR cluster
- """
+ """
response = self.get_conn().list_clusters(ClusterStates=cluster_states)
+ while true:
+ matching_clusters = list(
+ filter(lambda cluster: cluster["Name"] == emr_cluster_name, response["Clusters"])
+ )
- matching_clusters = list(
- filter(lambda cluster: cluster["Name"] == emr_cluster_name, response["Clusters"])
- )
-
- if len(matching_clusters) == 1:
- cluster_id = matching_clusters[0]["Id"]
- self.log.info("Found cluster name = %s id = %s", emr_cluster_name, cluster_id)
- return cluster_id
- elif len(matching_clusters) > 1:
- raise AirflowException(f"More than one cluster found for name {emr_cluster_name}")
- else:
- self.log.info("No cluster found for name %s", emr_cluster_name)
- return None
+ if len(matching_clusters) == 1:
Review Comment:
This logic isn't quite right, what if two clusters have the same name but are on different pages?
--
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
[GitHub] [airflow] vincbeck commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1126564027
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -191,6 +197,9 @@ def test_get_cluster_id_by_name(self):
assert no_match is None
+ with pytest.raises(AirflowException):
+ hook.get_cluster_id_by_name("test_cluster0", ["RUNNING", "WAITING", "BOOTSTRAPPING"])
+
Review Comment:
I dont think that's what @o-nikolas is saying. I think he wants to have a test to cover the use case **in case** the cluster you are looking for is in the second page. The implementation should not change, just adding a test that covers this use case would increase the testing coverage. And I agree with that
--
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
[GitHub] [airflow] potiuk commented on pull request #29732: Loop through paginated response to check for cluster id
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29732:
URL: https://github.com/apache/airflow/pull/29732#issuecomment-1528885560
unexpected ident does not signal anything good.
--
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
[GitHub] [airflow] o-nikolas commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1179751682
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -181,6 +182,11 @@ def test_get_cluster_id_by_name(self):
{"Name": "test_cluster", "Instances": {"KeepJobFlowAliveWhenNoSteps": True}}
)
+ for i in range(51):
+ hook.create_job_flow(
+ {"Name": "test_cluster" + str(i % 50), "Instances": {"KeepJobFlowAliveWhenNoSteps": True}}
Review Comment:
Addressed
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -191,6 +197,9 @@ def test_get_cluster_id_by_name(self):
assert no_match is None
+ with pytest.raises(AirflowException):
+ hook.get_cluster_id_by_name("test_cluster0", ["RUNNING", "WAITING", "BOOTSTRAPPING"])
+
Review Comment:
Addressed
--
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
[GitHub] [airflow] thirtyseven commented on pull request #29732: Loop through paginated response to check for cluster id
Posted by "thirtyseven (via GitHub)" <gi...@apache.org>.
thirtyseven commented on PR #29732:
URL: https://github.com/apache/airflow/pull/29732#issuecomment-1454161380
@nsAstro I am not a codeowner, you'll need one of @eladkal @o-nikolas to review.
--
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
[GitHub] [airflow] o-nikolas commented on pull request #29732: Loop through paginated response to check for cluster id
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #29732:
URL: https://github.com/apache/airflow/pull/29732#issuecomment-1529358523
> unexpected ident does not signal anything good.
Ooop! I updated @nsAstro's PR through the Github UI text editor (which has no bells and whistles for linting or catching those indents) so I missed that one! Hopefully that stops the build from timing out :grin:
--
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
[GitHub] [airflow] eladkal merged pull request #29732: Loop through paginated response to check for cluster id
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal merged PR #29732:
URL: https://github.com/apache/airflow/pull/29732
--
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
[GitHub] [airflow] o-nikolas commented on pull request #29732: Loop through paginated response to check for cluster id
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #29732:
URL: https://github.com/apache/airflow/pull/29732#issuecomment-1530058001
Looks like this is all passing and we have a couple of approvals. I'm not sure if it's proper for me to approve and merge this since the PR is mostly my code at this point, so CCing @potiuk and @eladkal to approve and merge (and/or advise that it's okay if I do).
--
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
[GitHub] [airflow] o-nikolas commented on pull request #29732: Loop through paginated response to check for cluster id
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #29732:
URL: https://github.com/apache/airflow/pull/29732#issuecomment-1509362627
Hey @nsAstro! Any status update on this one?
Seems like just a few small outstanding feedbacks.
--
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
[GitHub] [airflow] nsAstro commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "nsAstro (via GitHub)" <gi...@apache.org>.
nsAstro commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1124519620
##########
airflow/providers/amazon/aws/hooks/emr.py:
##########
@@ -68,22 +68,24 @@ def get_cluster_id_by_name(self, emr_cluster_name: str, cluster_states: list[str
:param emr_cluster_name: Name of a cluster to find
:param cluster_states: State(s) of cluster to find
:return: id of the EMR cluster
- """
+ """
response = self.get_conn().list_clusters(ClusterStates=cluster_states)
+ while true:
+ matching_clusters = list(
+ filter(lambda cluster: cluster["Name"] == emr_cluster_name, response["Clusters"])
+ )
- matching_clusters = list(
- filter(lambda cluster: cluster["Name"] == emr_cluster_name, response["Clusters"])
- )
-
- if len(matching_clusters) == 1:
- cluster_id = matching_clusters[0]["Id"]
- self.log.info("Found cluster name = %s id = %s", emr_cluster_name, cluster_id)
- return cluster_id
- elif len(matching_clusters) > 1:
- raise AirflowException(f"More than one cluster found for name {emr_cluster_name}")
- else:
- self.log.info("No cluster found for name %s", emr_cluster_name)
- return None
+ if len(matching_clusters) == 1:
+ cluster_id = matching_clusters[0]["Id"]
+ self.log.info("Found cluster name = %s id = %s", emr_cluster_name, cluster_id)
+ return cluster_id
+ elif len(matching_clusters) > 1:
+ raise AirflowException(f"More than one cluster found for name {emr_cluster_name}")
+ elif response["Marker"]:
Review Comment:
Yeah, I was optimizing for speed when I thought we were filtering on a unique ID. Just rewrote to use the above.
--
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
[GitHub] [airflow] nsAstro commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "nsAstro (via GitHub)" <gi...@apache.org>.
nsAstro commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1124518596
##########
airflow/providers/amazon/aws/hooks/emr.py:
##########
@@ -68,22 +68,24 @@ def get_cluster_id_by_name(self, emr_cluster_name: str, cluster_states: list[str
:param emr_cluster_name: Name of a cluster to find
:param cluster_states: State(s) of cluster to find
:return: id of the EMR cluster
- """
+ """
response = self.get_conn().list_clusters(ClusterStates=cluster_states)
+ while true:
+ matching_clusters = list(
+ filter(lambda cluster: cluster["Name"] == emr_cluster_name, response["Clusters"])
+ )
- matching_clusters = list(
- filter(lambda cluster: cluster["Name"] == emr_cluster_name, response["Clusters"])
- )
-
- if len(matching_clusters) == 1:
- cluster_id = matching_clusters[0]["Id"]
- self.log.info("Found cluster name = %s id = %s", emr_cluster_name, cluster_id)
- return cluster_id
- elif len(matching_clusters) > 1:
- raise AirflowException(f"More than one cluster found for name {emr_cluster_name}")
- else:
- self.log.info("No cluster found for name %s", emr_cluster_name)
- return None
+ if len(matching_clusters) == 1:
Review Comment:
Addressed this in latest commit. I missed that cluster_name was non-unique.
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1128979577
##########
tests/providers/amazon/aws/hooks/test_emr.py:
##########
@@ -174,13 +175,22 @@ def test_emr_connection(self, aws_conn_id, emr_conn_id):
def test_get_cluster_id_by_name(self):
"""
Test that we can resolve cluster id by cluster name.
+ Create 50 clusters with the range 1-50 appended and create a duplicate of index 1
+ to test logic.
Review Comment:
```suggestion
Create 50 clusters with the range 1-50 appended and create a duplicate of index 1
to test logic.
```
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #29732: Loop through paginated response to check for cluster id
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29732:
URL: https://github.com/apache/airflow/pull/29732#discussion_r1128979153
##########
airflow/providers/amazon/aws/hooks/emr.py:
##########
@@ -69,11 +69,14 @@ def get_cluster_id_by_name(self, emr_cluster_name: str, cluster_states: list[str
:param cluster_states: State(s) of cluster to find
:return: id of the EMR cluster
"""
- response = self.get_conn().list_clusters(ClusterStates=cluster_states)
-
- matching_clusters = list(
- filter(lambda cluster: cluster["Name"] == emr_cluster_name, response["Clusters"])
+ response_iterator = (
+ self.get_conn().get_paginator("list_clusters").paginate(ClusterStates=cluster_states)
)
+ matching_clusters = []
+ for page in response_iterator:
+ matching_clusters.extend(
+ list(filter(lambda cluster: cluster["Name"] == emr_cluster_name, page["Clusters"]))
+ )
Review Comment:
```suggestion
matching_clusters = [
cluster
for page in response_iterator
for cluster in page["Clusters"]
if cluster["Name"] == emr_cluster_name
]
```
--
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