') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); [v2-11-test] Resolve OOM When Reading Large Logs in Webserver by jason810496 · Pull Request #45914 · apache/airflow · GitHub
Skip to content

[v2-11-test] Resolve OOM When Reading Large Logs in Webserver - #45914

Closed
jason810496 wants to merge 11 commits into
apache:v2-11-testfrom
jason810496:refactor/webserver-oom-for-large-log-read-back-port
Closed

[v2-11-test] Resolve OOM When Reading Large Logs in Webserver#45914
jason810496 wants to merge 11 commits into
apache:v2-11-testfrom
jason810496:refactor/webserver-oom-for-large-log-read-back-port

Conversation

@jason810496

@jason810496jason810496 commented Jan 22, 2025

Copy link
Copy Markdown
Member

related issue: #45079
related PR: #45129
related discussion on slack: https://apache-airflow.slack.com/archives/CCZRF2U5A/p1736767159693839

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborgboring-cyborgBot added area:API Airflow's REST/HTTP API area:logging area:webserver Webserver related Issues labels Jan 22, 2025
@Lee-WLee-W added the legacy api Whether legacy API changes should be allowed in PR label Jan 22, 2025
@jason810496
jason810496force-pushed the refactor/webserver-oom-for-large-log-read-back-port branch 5 times, most recently from cc217a5 to e9e1891CompareJanuary 23, 2025 11:34
@jason810496
jason810496 marked this pull request as ready for review January 24, 2025 03:04
@potiukpotiuk mentioned this pull request Jan 27, 2025
Merged
@jason810496
jason810496force-pushed the refactor/webserver-oom-for-large-log-read-back-port branch from f08efca to 2c1a3ccCompareJanuary 29, 2025 14:54
potiuk
potiuk approved these changes Apr 29, 2025
View reviewed changes

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great - few nits, but I think this is a great improvement.

@potiuk

potiuk commented Apr 29, 2025

Copy link
Copy Markdown
Member

I marked it as 3.0.1 - because technically - it is bugfix, though we might refrain from merging it to 3.0.1 - up to @kaxil to decide, but I would be inclined to include it, especially if we decide to also release it in 2.11 - it would be strange to have it in 2.11.0 but not have it in 3.0.*

@jason810496

jason810496 commented Apr 29, 2025•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Hi @potiuk, #49470 targets the 3.0+ branch, since there are significant changes to FileTaskHandler introduced in Airflow 3.0, as detailed in the #49470 description. ( introduce StructuredLogMessage and RemoteIO to FileTaskHandler in 3.0 )

Backporting this one (#45914) directly would likely result in conflicts. However, it should be no problem to backport #49470 to the v3-0-test branch instead.

@jason810496
jason810496force-pushed the refactor/webserver-oom-for-large-log-read-back-port branch 2 times, most recently from 133624d to 09799f4CompareApril 30, 2025 12:43
@kaxil
kaxil changed the base branch from v2-10-test to v2-11-testApril 30, 2025 18:58
@kaxilkaxil changed the title [v2-10-test] Resolve OOM When Reading Large Logs in Webserver[v2-11-test] Resolve OOM When Reading Large Logs in WebserverApr 30, 2025
dependabotBotand others added 10 commits May 1, 2025 00:28
@dependabot@kaxil
Bump trove-classifiers from 2025.4.11.15 to 2025.4.28.22 (apache#49973)
7fe9b77
Bumps [trove-classifiers](https://github.com/pypa/trove-classifiers) from 2025.4.11.15 to 2025.4.28.22.
- [Release notes](https://github.com/pypa/trove-classifiers/releases)
- [Commits](pypa/trove-classifiers@2025.4.11.15...2025.4.28.22)
---
updated-dependencies:
- dependency-name: trove-classifiers
dependency-version: 2025.4.28.22
dependency-type: direct:development
update-type: version-update:semver-patch
...
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@jason810496@kaxil
[v2-10-test] Resolve OOM When Reading Large Logs in Webserver
ab176bf
@jason810496@kaxil
Fix test decorator
3226233
@jason810496@kaxil
Fix test
680a713
@jason810496@kaxil
Fix test_log_handler, test_log_reader
48debfe
@jason810496@kaxil
Fix scheduler_job_runner
54386ec
@jason810496@kaxil
Fix log_handler
c6d4e5e
- test_interleave_interleaves
- test_interleave_logs_correct_ordering
- test__read_for_celery_executor_fallbacks_to_worker
@jason810496@kaxil
Fix test_log_endpoint test
7e39433
@jason810496@kaxil
fixup! Add type hint
5797341
@jason810496@kaxil
fixup! remove unnecessary type hint
e4e5a85
@kaxil
kaxilforce-pushed the refactor/webserver-oom-for-large-log-read-back-port branch from 09799f4 to 246e4fbCompareApril 30, 2025 18:58
kaxil
kaxil reviewed Apr 30, 2025
View reviewed changes
Comment threadairflow/jobs/scheduler_job_runner.py
message = str(next(iter(log_streams[0])))
while metadata["end_of_log"] is False:
logs, metadata = task_log_reader.read_log_chunks(
hosts, log_streams, metadata = task_log_reader.read_log_chunks(

@kaxilkaxilApr 30, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this is a big change, I am afraid to include it in 2.11!

For example, in this case where the return type of task_log_reader.read_log_chunks has changed.

@dstandishdstandishApr 30, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kaxil it seems he is intending this to go to 3.0 only
#45914 (comment)

@dstandishdstandishApr 30, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah we should not spend time adding meaningful changes to 2.x

@jason810496jason810496May 1, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

For example, in this case where the return type of task_log_reader.read_log_chunks has changed.

I think changing the return type of read_log_chunks or other internal methods isn't a problem, since the user-facing interface is the REST API. As long as the REST API response remains consistent, users won’t be affected or need to be aware of the refactor.

kaxil
kaxil reviewed Apr 30, 2025
View reviewed changes
Comment threadairflow/utils/serve_logs.py
@kaxil

kaxil commented Apr 30, 2025

Copy link
Copy Markdown
Member

Great PR @jason810496 but I am worried about backwards compatibility with this change -- especially for cases where an external provider has built upon the 2.10.5 logging format or clients using /log endpoint with json.

cc @dstandish@uranusjr@ashb Any opinion from you folks since you are more familiar with our logging architecture? My recommendation is to not include this in 2.x and close this and review #49470 for 3.1

@jason810496
jason810496force-pushed the refactor/webserver-oom-for-large-log-read-back-port branch from 246e4fb to 6cef899CompareMay 1, 2025 05:20
@jason810496

jason810496 commented May 1, 2025•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Great PR @jason810496 but I am worried about backwards compatibility with this change -- especially for cases where an external provider has built upon the 2.10.5 logging format or clients using /log endpoint with json.

Just clarify this change about break backward compatibility for log serving.

cc @dstandish@uranusjr@ashb Any opinion from you folks since you are more familiar with our logging architecture? My recommendation is to not include this in 2.x and close this and review #49470 for 3.1

That works for me as well.

Thanks for the review, @potiuk and @kaxil!

@kaxilkaxil removed this from the Airflow 3.0.1 milestone May 6, 2025
@kaxilkaxil closed this May 6, 2025
@kaxil

kaxil commented May 6, 2025

Copy link
Copy Markdown
Member

Thanks @jason810496 , will review #49470

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@uranusjruranusjruranusjr left review comments
@kaxilkaxilkaxil requested changes
@potiukpotiukpotiuk approved these changes
@eladkaleladkalAwaiting requested review from eladkaleladkal is a code owner
@o-nikolaso-nikolasAwaiting requested review from o-nikolaso-nikolas is a code owner
@ryanahamiltonryanahamiltonAwaiting requested review from ryanahamiltonryanahamilton is a code owner
@ashbashbAwaiting requested review from ashbashb is a code owner
@bbovenzibbovenziAwaiting requested review from bbovenzibbovenzi is a code owner
@pierrejeambrunpierrejeambrunAwaiting requested review from pierrejeambrunpierrejeambrun is a code owner
@ephraimbuddyephraimbuddyAwaiting requested review from ephraimbuddyephraimbuddy is a code owner
@XD-DENGXD-DENGAwaiting requested review from XD-DENGXD-DENG is a code owner
@bolkedebruinbolkedebruinAwaiting requested review from bolkedebruin
@jedcunninghamjedcunninghamAwaiting requested review from jedcunningham
@dstandishdstandishAwaiting requested review from dstandish
@FokkoFokkoAwaiting requested review from Fokko
@mik-lajmik-lajAwaiting requested review from mik-laj
@hussein-awalahussein-awalaAwaiting requested review from hussein-awalahussein-awala is a code owner
@josh-felljosh-fellAwaiting requested review from josh-fell
@jhtimminsjhtimminsAwaiting requested review from jhtimmins
@Lee-WLee-WAwaiting requested review from Lee-W

Assignees

No one assigned

Labels

area:APIAirflow's REST/HTTP APIarea:loggingarea:webserverWebserver related Issueslegacy apiWhether legacy API changes should be allowed in PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jason810496@potiuk@kaxil@uranusjr@dstandish@Lee-W