[GitHub] [airflow] akki commented on a change in pull request #6552: [AIRFLOW-5850] Capture task logs in DockerSwarmOperator
akki commented on a change in pull request #6552: [AIRFLOW-5850] Capture task
logs in DockerSwarmOperator
URL: https://github.com/apache/airflow/pull/6552#discussion_r382924991
##
File path: airflow/contrib/operators/docker_swarm_operator.py
##
@@ -123,11 +129,43 @@ def _run_image(self):
self.log.info('Service started: %s', str(self.service))
-status = None
# wait for the service to start the task
while not self.cli.tasks(filters={'service': self.service['ID']}):
continue
-while True:
+
+logs = self.cli.service_logs(
+self.service['ID'], follow=True, stdout=True, stderr=True,
is_tty=self.tty
+)
+line = ''
+_stream_logs = self.enable_logging # Status of the service_logs'
generator
+while True: # pylint: disable=too-many-nested-blocks
+if self.enable_logging:
Review comment:
@dimberman I have addressed this now - please have a look.
Thanks again for your suggestion.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] akki commented on a change in pull request #6552: [AIRFLOW-5850] Capture task logs in DockerSwarmOperator
akki commented on a change in pull request #6552: [AIRFLOW-5850] Capture task
logs in DockerSwarmOperator
URL: https://github.com/apache/airflow/pull/6552#discussion_r382924991
##
File path: airflow/contrib/operators/docker_swarm_operator.py
##
@@ -123,11 +129,43 @@ def _run_image(self):
self.log.info('Service started: %s', str(self.service))
-status = None
# wait for the service to start the task
while not self.cli.tasks(filters={'service': self.service['ID']}):
continue
-while True:
+
+logs = self.cli.service_logs(
+self.service['ID'], follow=True, stdout=True, stderr=True,
is_tty=self.tty
+)
+line = ''
+_stream_logs = self.enable_logging # Status of the service_logs'
generator
+while True: # pylint: disable=too-many-nested-blocks
+if self.enable_logging:
Review comment:
@dimberman I have addressed this now - please have a look.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] akki commented on a change in pull request #6552: [AIRFLOW-5850] Capture task logs in DockerSwarmOperator
akki commented on a change in pull request #6552: [AIRFLOW-5850] Capture task
logs in DockerSwarmOperator
URL: https://github.com/apache/airflow/pull/6552#discussion_r382924952
##
File path: airflow/providers/docker/operators/docker_swarm.py
##
@@ -123,11 +129,43 @@ def _run_image(self):
self.log.info('Service started: %s', str(self.service))
-status = None
# wait for the service to start the task
while not self.cli.tasks(filters={'service': self.service['ID']}):
continue
-while True:
+
+logs = self.cli.service_logs(
+self.service['ID'], follow=True, stdout=True, stderr=True,
is_tty=self.tty
+)
+line = ''
+_stream_logs = self.enable_logging # Status of the service_logs'
generator
+while True: # pylint: disable=too-many-nested-blocks
+if self.enable_logging:
Review comment:
@mik-laj Thank you for the suggestions for improvement.
I have fixed these now. Please see if they look good to you.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] akki commented on a change in pull request #6552: [AIRFLOW-5850] Capture task logs in DockerSwarmOperator
akki commented on a change in pull request #6552: [AIRFLOW-5850] Capture task
logs in DockerSwarmOperator
URL: https://github.com/apache/airflow/pull/6552#discussion_r371607250
##
File path: airflow/providers/docker/operators/docker_swarm.py
##
@@ -123,11 +129,43 @@ def _run_image(self):
self.log.info('Service started: %s', str(self.service))
-status = None
# wait for the service to start the task
while not self.cli.tasks(filters={'service': self.service['ID']}):
continue
-while True:
+
+logs = self.cli.service_logs(
+self.service['ID'], follow=True, stdout=True, stderr=True,
is_tty=self.tty
+)
+line = ''
+_stream_logs = self.enable_logging # Status of the service_logs'
generator
+while True: # pylint: disable=too-many-nested-blocks
+if self.enable_logging:
Review comment:
Okay, I can think of a way to simplify the logic.
(By putting the service termination check within the `except
ConnectionError` clause itself.)
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task logs in DockerSwarmOperator
akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task
logs in DockerSwarmOperator
URL: https://github.com/apache/airflow/pull/6552#discussion_r361391339
##
File path: airflow/contrib/operators/docker_swarm_operator.py
##
@@ -123,11 +129,43 @@ def _run_image(self):
self.log.info('Service started: %s', str(self.service))
-status = None
# wait for the service to start the task
while not self.cli.tasks(filters={'service': self.service['ID']}):
continue
-while True:
+
+logs = self.cli.service_logs(
+self.service['ID'], follow=True, stdout=True, stderr=True,
is_tty=self.tty
+)
+line = ''
+_stream_logs = self.enable_logging # Status of the service_logs'
generator
+while True: # pylint: disable=too-many-nested-blocks
+if self.enable_logging:
Review comment:
@dimberman Does this work for you? Or do you have any other suggestions?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task logs in DockerSwarmOperator
akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task
logs in DockerSwarmOperator
URL: https://github.com/apache/airflow/pull/6552#discussion_r358642854
##
File path: airflow/contrib/operators/docker_swarm_operator.py
##
@@ -123,11 +129,43 @@ def _run_image(self):
self.log.info('Service started: %s', str(self.service))
-status = None
# wait for the service to start the task
while not self.cli.tasks(filters={'service': self.service['ID']}):
continue
-while True:
+
+logs = self.cli.service_logs(
+self.service['ID'], follow=True, stdout=True, stderr=True,
is_tty=self.tty
+)
+line = ''
+_stream_logs = self.enable_logging # Status of the service_logs'
generator
+while True: # pylint: disable=too-many-nested-blocks
+if self.enable_logging:
Review comment:
Added a comment to the code, hope that improves the readability.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task logs in DockerSwarmOperator
akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task
logs in DockerSwarmOperator
URL: https://github.com/apache/airflow/pull/6552#discussion_r358642854
##
File path: airflow/contrib/operators/docker_swarm_operator.py
##
@@ -123,11 +129,43 @@ def _run_image(self):
self.log.info('Service started: %s', str(self.service))
-status = None
# wait for the service to start the task
while not self.cli.tasks(filters={'service': self.service['ID']}):
continue
-while True:
+
+logs = self.cli.service_logs(
+self.service['ID'], follow=True, stdout=True, stderr=True,
is_tty=self.tty
+)
+line = ''
+_stream_logs = self.enable_logging # Status of the service_logs'
generator
+while True: # pylint: disable=too-many-nested-blocks
+if self.enable_logging:
Review comment:
Added a comment to the code, if that helps with the readability.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task logs in DockerSwarmOperator
akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task
logs in DockerSwarmOperator
URL: https://github.com/apache/airflow/pull/6552#discussion_r358641668
##
File path: airflow/contrib/operators/docker_swarm_operator.py
##
@@ -123,11 +129,43 @@ def _run_image(self):
self.log.info('Service started: %s', str(self.service))
-status = None
# wait for the service to start the task
while not self.cli.tasks(filters={'service': self.service['ID']}):
continue
-while True:
+
+logs = self.cli.service_logs(
+self.service['ID'], follow=True, stdout=True, stderr=True,
is_tty=self.tty
+)
+line = ''
+_stream_logs = self.enable_logging # Status of the service_logs'
generator
+while True: # pylint: disable=too-many-nested-blocks
+if self.enable_logging:
Review comment:
I had tried that earlier but this whole block makes sense only if put
together. The `while True` has 2 responsibilities - log things and check if the
service still running - but I don't see any straightforward way to decouple
them.
Also, this block waits for the service to stop which doesn't make sens
without the part where the service is started.
Do you have any particular ideas in mind?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task logs in DockerSwarmOperator
akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task
logs in DockerSwarmOperator
URL: https://github.com/apache/airflow/pull/6552#discussion_r348655412
##
File path: airflow/contrib/operators/docker_swarm_operator.py
##
@@ -127,7 +133,37 @@ def _run_image(self):
# wait for the service to start the task
while not self.cli.tasks(filters={'service': self.service['ID']}):
continue
+
+logs = self.cli.service_logs(self.service['ID'], follow=True,
stdout=True, stderr=True, is_tty=self.tty)
+line = ''
+_stream_logs = self.enable_logging # Status of the service_logs'
generator
while True:
Review comment:
Ahh.. if removing this patch doesn't work either for you then it should be
something else. Marking this as resolved but let me know if u need any help
debugging the issue.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task logs in DockerSwarmOperator
akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task
logs in DockerSwarmOperator
URL: https://github.com/apache/airflow/pull/6552#discussion_r347103996
##
File path: airflow/contrib/operators/docker_swarm_operator.py
##
@@ -127,7 +133,37 @@ def _run_image(self):
# wait for the service to start the task
while not self.cli.tasks(filters={'service': self.service['ID']}):
continue
+
+logs = self.cli.service_logs(self.service['ID'], follow=True,
stdout=True, stderr=True, is_tty=self.tty)
+line = ''
+_stream_logs = self.enable_logging # Status of the service_logs'
generator
while True:
Review comment:
Can you try with `command=sleep 10` maybe?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task logs in DockerSwarmOperator
akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task logs in DockerSwarmOperator URL: https://github.com/apache/airflow/pull/6552#discussion_r347103956 ## File path: airflow/contrib/operators/docker_swarm_operator.py ## @@ -89,17 +90,22 @@ class DockerSwarmOperator(DockerOperator): :param tty: Allocate pseudo-TTY to the container of this service This needs to be set see logs of the Docker container / service. :type tty: bool +:param enable_logging: Show the application's logs in operator's logs. +Supported only if the Docker engine is using json-file or journald logging drivers. +The `tty` parameter should be set to use this with Python applications. +:type enable_logging: bool """ @apply_defaults def __init__( self, image, +enable_logging=True, *args, **kwargs): - super().__init__(image=image, *args, **kwargs) +self.enable_logging = enable_logging self.service = None def _run_image(self): Review comment: Airflow admins would probably require a new ticket for this. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [airflow] akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task logs in DockerSwarmOperator
akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task logs in DockerSwarmOperator URL: https://github.com/apache/airflow/pull/6552#discussion_r347103945 ## File path: airflow/contrib/operators/docker_swarm_operator.py ## @@ -89,17 +90,22 @@ class DockerSwarmOperator(DockerOperator): :param tty: Allocate pseudo-TTY to the container of this service This needs to be set see logs of the Docker container / service. :type tty: bool +:param enable_logging: Show the application's logs in operator's logs. +Supported only if the Docker engine is using json-file or journald logging drivers. +The `tty` parameter should be set to use this with Python applications. +:type enable_logging: bool """ @apply_defaults def __init__( self, image, +enable_logging=True, *args, **kwargs): - super().__init__(image=image, *args, **kwargs) +self.enable_logging = enable_logging self.service = None def _run_image(self): Review comment: The test never actually calls the API, so I think it won't catch this. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [airflow] akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task logs in DockerSwarmOperator
akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task logs in DockerSwarmOperator URL: https://github.com/apache/airflow/pull/6552#discussion_r347103744 ## File path: airflow/contrib/operators/docker_swarm_operator.py ## @@ -127,7 +133,37 @@ def _run_image(self): # wait for the service to start the task Review comment: Looks like it was never needed in the first place. 樂 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [airflow] akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task logs in DockerSwarmOperator
akki commented on a change in pull request #6552: AIRFLOW-5850: Capture task
logs in DockerSwarmOperator
URL: https://github.com/apache/airflow/pull/6552#discussion_r347101988
##
File path: airflow/contrib/operators/docker_swarm_operator.py
##
@@ -127,7 +133,37 @@ def _run_image(self):
# wait for the service to start the task
while not self.cli.tasks(filters={'service': self.service['ID']}):
continue
+
+logs = self.cli.service_logs(self.service['ID'], follow=True,
stdout=True, stderr=True, is_tty=self.tty)
+line = ''
+_stream_logs = self.enable_logging # Status of the service_logs'
generator
while True:
Review comment:
Does it go away when you remove this commit? Does it go away if you set
`enable_loggin=False`?
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:
us...@infra.apache.org
With regards,
Apache Git Services
