Skip to content

Conversation

@dstandish
Copy link
Contributor

We were passing around the logs variable (actually PodLogConsumer object) for no reason. And the termination_timeout param was not used.

@dstandish dstandish requested a review from pankajkoti November 3, 2023 18:11
@boring-cyborg boring-cyborg bot added area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues labels Nov 3, 2023
@dstandish dstandish requested review from jedcunningham and removed request for jedcunningham November 3, 2023 18:12
@dstandish dstandish force-pushed the remove-inconsequential-code-bits-kpo-logging branch from 6d6db6c to b0aa9c6 Compare November 3, 2023 18:14
@dstandish
Copy link
Contributor Author

marking as draft, need to review what was in #34127

@dstandish dstandish marked this pull request as draft November 3, 2023 18:30
@dstandish
Copy link
Contributor Author

OK first there was #34127 by @dkim010, which added the "reusing" of the PodLogsConsumer object.

But then after that came #34412 by @hussein-awala , which changed the handling of the object.

Now, it appears to me, that the forwarding of the logs object does not have any effect.

Inside consume_logs, a new consumer object is always created:
https://github.com/apache/airflow/pull/35416/files#diff-6900da9281d8404b14da0815f2b37350f3148b1b63449928b952744e6711e7e7L414

If that step fails, nothing is done with the consumer object. In that situation, the old consumer object would be returned along with last captured time; but then after that it would just be at most passed back in to consume_logs again, and ignored again.

Do you agree @hussein-awala and @dkim010? I think this PR is an appropriate cleanup.

KPO logging is now extremely convoluted. One avenue for cleanup is getting rid of some irrelevant follow= logic, which I'll try to do. Some of that was relevant only when triggers could not log, so we added periodic resuming and log capturing, which is not a feature in this operator anymore.

@dstandish dstandish marked this pull request as ready for review November 3, 2023 20:05
@dstandish dstandish force-pushed the remove-inconsequential-code-bits-kpo-logging branch 3 times, most recently from 13c09a3 to fcd3364 Compare November 4, 2023 01:57
@dkim010
Copy link
Contributor

dkim010 commented Nov 5, 2023

Do you agree @hussein-awala and @dkim010? I think this PR is an appropriate cleanup.

You are right. I agree with that #34412 made #34127 have no effect.

KPO logging is now extremely convoluted. One avenue for cleanup is getting rid of some irrelevant follow= logic, which I'll try to do. Some of that was relevant only when triggers could not log, so we added periodic resuming and log capturing, which is not a feature in this operator anymore.

Would you explain why the follow= logic is irrelevant to KPO logging?
And, I think the test_fetch_container_logs_failures test case (related to the follow= logic) should be passed.
Is the test case passed?

@dstandish
Copy link
Contributor Author

It's not all entirely irrelevant. But some of it is. I'll tag you in the appropriate pr

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that my PR made #34127 has no effect (by mistake), so we need to add a condition before reading the log to fix the previous PR instead of removing the whole fix.

@dstandish dstandish force-pushed the remove-inconsequential-code-bits-kpo-logging branch from 68ace0e to b4373e9 Compare November 7, 2023 15:05
@dstandish
Copy link
Contributor Author

dstandish commented Nov 7, 2023

@hussein-awala my cleanup in this PR actually does a little more that just remove the the passing around of the logs object.
I also remove unused argument termination_timeout and unnecessary param follow (in the consume func).

Since passing around the logs object presently has no effect, then it is still appropriate to remove IMHO.

My vote would be to merge this now, and then later if someone wants to reintroduce something along the lines of #34127, and with appropriate testing, they can do so after. This would make it cleaner to review anyway, I think.

We were passing around the `logs` variable (actually PodLogConsumer object) for no reason.  And the `termination_timeout` param was not used.
@dstandish dstandish force-pushed the remove-inconsequential-code-bits-kpo-logging branch from b4373e9 to 5a96834 Compare November 7, 2023 20:09
@dstandish dstandish merged commit 486ccba into apache:main Nov 13, 2023
@dstandish dstandish deleted the remove-inconsequential-code-bits-kpo-logging branch November 13, 2023 18:36
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Nov 20, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.8.0 milestone Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:cncf-kubernetes Kubernetes (k8s) provider related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants