Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[dagster-aws] make PipesCloudWatchMessageReader subclass PipesThreadedMessageReader #24809

Open
wants to merge 1 commit into
base: dagster-pipes-mutable-logreaders
Choose a base branch
from
Open

[dagster-aws] make PipesCloudWatchMessageReader subclass PipesThreadedMessageReader #24809

wants to merge 1 commit into from

Conversation

danielgafni
Copy link
Contributor

@danielgafni danielgafni commented Sep 26, 2024 •
edited
Loading

Summary & Motivation

PipesCloudWatchMessageReader can now read messages & logs in background and read from streams which are not known initially.

How I Tested These Changes

Changelog

Insert changelog entry or "NOCHANGELOG" here.

  • NEW (PipesCloudWatchMessageReader now reads logs in background threads)

@danielgafni Graphite App
Copy link
Contributor Author

danielgafni commented Sep 26, 2024 •
edited
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @danielgafni and the rest of your teammates on Graphite Graphite

@danielgafni danielgafni mentioned this pull request Sep 26, 2024
Open
1 task
@danielgafni danielgafni marked this pull request as ready for review September 26, 2024 17:25
@danielgafni danielgafni marked this pull request as draft September 26, 2024 22:16
@danielgafni danielgafni force-pushed the PipesCloudWatchMessageReader-threaded branch from 9b6f81e to 9952ed4 Compare September 27, 2024 08:49
@danielgafni danielgafni marked this pull request as ready for review September 27, 2024 09:02
@danielgafni danielgafni mentioned this pull request Sep 30, 2024
Open
1 task
@danielgafni danielgafni force-pushed the PipesCloudWatchMessageReader-threaded branch from 9952ed4 to ca70374 Compare September 30, 2024 08:23
@danielgafni danielgafni mentioned this pull request Sep 30, 2024
Open
2 tasks
@danielgafni danielgafni force-pushed the 09-13-_dagster-pipes_add_threadedpipesmessagereader_with_extra_runtime_params branch from 271a6b7 to 7f7242b Compare September 30, 2024 10:44
@danielgafni danielgafni force-pushed the PipesCloudWatchMessageReader-threaded branch 2 times, most recently from c2f1acb to 9fba0f7 Compare September 30, 2024 11:07
@danielgafni danielgafni force-pushed the PipesCloudWatchMessageReader-threaded branch from 9fba0f7 to e9be4b5 Compare September 30, 2024 15:48
@graphite-app graphite-app bot added the area: docs Related to documentation in general label Sep 30, 2024
@graphite-app Graphite App
Copy link

graphite-app bot commented Sep 30, 2024

Graphite Automations

"Label and add CE on all Docs" took an action on this PR • (09/30/24)

1 label was added and 3 reviewers were added to this PR based on Pedram Navid's automation.

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 30, 2024 •
edited
Loading

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-d1pq53i2f-elementl.vercel.app
https://PipesCloudWatchMessageReader-threaded.dagster.dagster-docs.io

Direct link to changed pages:

@danielgafni danielgafni force-pushed the PipesCloudWatchMessageReader-threaded branch from e9be4b5 to a839f8f Compare September 30, 2024 16:08
@danielgafni danielgafni force-pushed the 09-13-_dagster-pipes_add_threadedpipesmessagereader_with_extra_runtime_params branch from 7f7242b to d63500e Compare October 1, 2024 15:29
@danielgafni danielgafni mentioned this pull request Oct 1, 2024
Merged
@danielgafni danielgafni force-pushed the 09-13-_dagster-pipes_add_threadedpipesmessagereader_with_extra_runtime_params branch from 2ebe652 to cf54202 Compare October 1, 2024 20:37
@danielgafni danielgafni force-pushed the PipesCloudWatchMessageReader-threaded branch 2 times, most recently from 78e54b5 to cbe61ea Compare October 1, 2024 20:46
danielgafni
danielgafni commented Oct 1, 2024
View reviewed changes
python_modules/libraries/dagster-aws/dagster_aws/pipes/message_readers.py Outdated

def on_launched(self, params: PipesLaunchedData) -> None:
if "log_group" in params["extras"]:
self.log_group = params["extras"]["log_group"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smackesey seems like if we do something like this the can_start method doesn't even need to have params argument.

We can set the actual attributes in on_opened and on_launched.

No need to introduce another source of parameters.

What do you think?

@danielgafni danielgafni force-pushed the 09-13-_dagster-pipes_add_threadedpipesmessagereader_with_extra_runtime_params branch 2 times, most recently from c10d2ff to 681425a Compare October 2, 2024 14:28
@danielgafni danielgafni force-pushed the PipesCloudWatchMessageReader-threaded branch from cbe61ea to 077603f Compare October 2, 2024 14:29
@danielgafni danielgafni force-pushed the 09-13-_dagster-pipes_add_threadedpipesmessagereader_with_extra_runtime_params branch from 681425a to 4f6fcc0 Compare October 2, 2024 15:13
@danielgafni danielgafni force-pushed the PipesCloudWatchMessageReader-threaded branch from 077603f to daa0736 Compare October 2, 2024 15:14
@danielgafni danielgafni force-pushed the 09-13-_dagster-pipes_add_threadedpipesmessagereader_with_extra_runtime_params branch from 4f6fcc0 to d48132c Compare October 2, 2024 15:54
@danielgafni danielgafni force-pushed the PipesCloudWatchMessageReader-threaded branch from daa0736 to b80d4f9 Compare October 2, 2024 15:54
@danielgafni danielgafni mentioned this pull request Oct 2, 2024
Draft
3 tasks
@danielgafni danielgafni force-pushed the 09-13-_dagster-pipes_add_threadedpipesmessagereader_with_extra_runtime_params branch from d48132c to 4ef06eb Compare October 2, 2024 22:38
@danielgafni danielgafni force-pushed the PipesCloudWatchMessageReader-threaded branch from b80d4f9 to 8f9b419 Compare October 2, 2024 22:39
This was referenced Oct 3, 2024
Draft
Open
@danielgafni danielgafni force-pushed the 09-13-_dagster-pipes_add_threadedpipesmessagereader_with_extra_runtime_params branch from 5c6d2fa to 429be7c Compare October 4, 2024 14:51
@danielgafni danielgafni mentioned this pull request Oct 4, 2024
Open
1 task
@danielgafni danielgafni force-pushed the 09-13-_dagster-pipes_add_threadedpipesmessagereader_with_extra_runtime_params branch 2 times, most recently from 5b39999 to 8025f14 Compare October 4, 2024 15:38
@danielgafni danielgafni changed the base branch from 09-13-_dagster-pipes_add_threadedpipesmessagereader_with_extra_runtime_params to dagster-pipes-mutable-logreaders October 4, 2024 16:04
@danielgafni danielgafni force-pushed the dagster-pipes-mutable-logreaders branch from f9c7aac to e9402e9 Compare October 4, 2024 16:06
@danielgafni danielgafni force-pushed the PipesCloudWatchMessageReader-threaded branch from 8f9b419 to e7dba22 Compare October 4, 2024 16:06
@danielgafni danielgafni force-pushed the dagster-pipes-mutable-logreaders branch from e9402e9 to 156b7b5 Compare October 4, 2024 16:09
@danielgafni danielgafni force-pushed the PipesCloudWatchMessageReader-threaded branch from e7dba22 to ca0da98 Compare October 4, 2024 16:10
@danielgafni danielgafni force-pushed the dagster-pipes-mutable-logreaders branch from 156b7b5 to f7fc074 Compare October 4, 2024 16:15
@danielgafni danielgafni force-pushed the PipesCloudWatchMessageReader-threaded branch from ca0da98 to f67a22b Compare October 4, 2024 16:16
@danielgafni danielgafni force-pushed the dagster-pipes-mutable-logreaders branch from f7fc074 to 7627d2e Compare October 4, 2024 17:17
@danielgafni danielgafni force-pushed the PipesCloudWatchMessageReader-threaded branch from f67a22b to eb29a02 Compare October 4, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smackesey smackesey Awaiting requested review from smackesey

@schrockn schrockn Awaiting requested review from schrockn

Assignees
No one assigned
Labels
area: docs Related to documentation in general
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@danielgafni @schrockn