You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/10/07 16:02:49 UTC
[GitHub] [airflow] dimberman opened a new pull request #11330: Allow multiple schedulers in helm chart
dimberman opened a new pull request #11330:
URL: https://github.com/apache/airflow/pull/11330
(cherry picked from commit 7f06d6220747151f527ab372b6ce431c61f6f20f)
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on a change in pull request #11330: Allow multiple schedulers in helm chart
Posted by GitBox <gi...@apache.org>.
dimberman commented on a change in pull request #11330:
URL: https://github.com/apache/airflow/pull/11330#discussion_r501839510
##########
File path: chart/templates/scheduler/scheduler-deployment.yaml
##########
@@ -46,7 +46,7 @@ spec:
{{- if $stateful }}
serviceName: {{ .Release.Name }}-scheduler
{{- end }}
- replicas: 1
+ replicas: {{ .Values.scheduler.replicas }}
Review comment:
@ashb yeah I don't think there's a warning, maybe we just need to be clear in documentation :/.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman merged pull request #11330: Allow multiple schedulers in helm chart
Posted by GitBox <gi...@apache.org>.
dimberman merged pull request #11330:
URL: https://github.com/apache/airflow/pull/11330
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11330: Allow multiple schedulers in helm chart
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11330:
URL: https://github.com/apache/airflow/pull/11330#discussion_r501807593
##########
File path: chart/values.yaml
##########
@@ -288,6 +288,7 @@ workers:
# Airflow scheduler settings
scheduler:
+ replicas: 2
Review comment:
```suggestion
replicas: 1
```
Default needs to stay at 1 I think
##########
File path: chart/values.yaml
##########
@@ -28,10 +28,10 @@ gid: 50000
airflowHome: "/opt/airflow"
# Default airflow repository -- overrides all the specific images below
-defaultAirflowRepository: apache/airflow
+defaultAirflowRepository: dimberman/airflow
# Default airflow tag to deploy
-defaultAirflowTag: 1.10.12
+defaultAirflowTag: scheduler-ha-with-dags-0.1
Review comment:
Need to revert these :)
##########
File path: chart/templates/scheduler/scheduler-deployment.yaml
##########
@@ -46,7 +46,7 @@ spec:
{{- if $stateful }}
serviceName: {{ .Release.Name }}-scheduler
{{- end }}
- replicas: 1
+ replicas: {{ .Values.scheduler.replicas }}
Review comment:
I wonder if there's any way we can guard this to only work for 2.0.0 -- but given users might use their own Docker image I don't think there's much we can really do.
I guess just make it clear in the values.yaml file do not set this >1 unless you are on 2.0.0.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11330: Allow multiple schedulers in helm chart
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11330:
URL: https://github.com/apache/airflow/pull/11330#discussion_r503386460
##########
File path: chart/values.yaml
##########
@@ -289,6 +289,7 @@ workers:
# Airflow scheduler settings
scheduler:
+ replicas: 1
Review comment:
Could you add a comment here about Mysql5 or Airflow 1.10
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on a change in pull request #11330: Allow multiple schedulers in helm chart
Posted by GitBox <gi...@apache.org>.
dimberman commented on a change in pull request #11330:
URL: https://github.com/apache/airflow/pull/11330#discussion_r501839510
##########
File path: chart/templates/scheduler/scheduler-deployment.yaml
##########
@@ -46,7 +46,7 @@ spec:
{{- if $stateful }}
serviceName: {{ .Release.Name }}-scheduler
{{- end }}
- replicas: 1
+ replicas: {{ .Values.scheduler.replicas }}
Review comment:
@ashb yeah I don't think there's a warning, maybe we just need to be clear in documentation :/.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #11330: Allow multiple schedulers in helm chart
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #11330:
URL: https://github.com/apache/airflow/pull/11330#issuecomment-721962277
This change seems to me not enough to support HA. We should also add an improvement to the liveness probe because it currently only watches the newest job, not the job assigned to the current scheduler.
https://github.com/apache/airflow/blob/91a64db505e50712cd53928b4f2b84aece3cc1c0/chart/templates/scheduler/scheduler-deployment.yaml#L115-L130
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11330: Allow multiple schedulers in helm chart
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11330:
URL: https://github.com/apache/airflow/pull/11330#discussion_r501807593
##########
File path: chart/values.yaml
##########
@@ -288,6 +288,7 @@ workers:
# Airflow scheduler settings
scheduler:
+ replicas: 2
Review comment:
```suggestion
replicas: 1
```
Default needs to stay at 1 I think
##########
File path: chart/values.yaml
##########
@@ -28,10 +28,10 @@ gid: 50000
airflowHome: "/opt/airflow"
# Default airflow repository -- overrides all the specific images below
-defaultAirflowRepository: apache/airflow
+defaultAirflowRepository: dimberman/airflow
# Default airflow tag to deploy
-defaultAirflowTag: 1.10.12
+defaultAirflowTag: scheduler-ha-with-dags-0.1
Review comment:
Need to revert these :)
##########
File path: chart/templates/scheduler/scheduler-deployment.yaml
##########
@@ -46,7 +46,7 @@ spec:
{{- if $stateful }}
serviceName: {{ .Release.Name }}-scheduler
{{- end }}
- replicas: 1
+ replicas: {{ .Values.scheduler.replicas }}
Review comment:
I wonder if there's any way we can guard this to only work for 2.0.0 -- but given users might use their own Docker image I don't think there's much we can really do.
I guess just make it clear in the values.yaml file do not set this >1 unless you are on 2.0.0.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on pull request #11330: Allow multiple schedulers in helm chart
Posted by GitBox <gi...@apache.org>.
dimberman commented on pull request #11330:
URL: https://github.com/apache/airflow/pull/11330#issuecomment-707214797
@kaxil updated
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on a change in pull request #11330: Allow multiple schedulers in helm chart
Posted by GitBox <gi...@apache.org>.
dimberman commented on a change in pull request #11330:
URL: https://github.com/apache/airflow/pull/11330#discussion_r503388616
##########
File path: chart/values.yaml
##########
@@ -289,6 +289,7 @@ workers:
# Airflow scheduler settings
scheduler:
+ replicas: 1
Review comment:
@ashb fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on a change in pull request #11330: Allow multiple schedulers in helm chart
Posted by GitBox <gi...@apache.org>.
dimberman commented on a change in pull request #11330:
URL: https://github.com/apache/airflow/pull/11330#discussion_r503386800
##########
File path: chart/values.yaml
##########
@@ -289,6 +289,7 @@ workers:
# Airflow scheduler settings
scheduler:
+ replicas: 1
Review comment:
Sure thing
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org