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.

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

feat!: create IceFlow nodes based on application DAG #109

Merged
merged 1 commit into from
Sep 17, 2024
Merged

Conversation

JKRhb
Copy link
Contributor

@JKRhb JKRhb commented Sep 11, 2024

This is the follow-up PR to #107, which attempts to perform an overhaul of the IceFlow API using our application DAG. This PR is still heavily WIP and gradually incorporate further changes made to #107.

@JKRhb JKRhb force-pushed the iceflow-dag-api branch 5 times, most recently from 6b07de9 to 47e3d1f Compare September 13, 2024 12:21
@JKRhb JKRhb requested review from tanim-ics and pulsastrix and removed request for tanim-ics September 13, 2024 12:27
@JKRhb JKRhb marked this pull request as ready for review September 13, 2024 12:27
@JKRhb
Copy link
Contributor Author

JKRhb commented Sep 13, 2024

I think this PR should now be ready for an initial review. The changes might have turned a bit more drastic (and in some places even controversial) than I originally intended, but I think this should point into the right direction.

Some of the most important aspects:

  • I removed any kind of publish internal and even the internal consumer queue, so data is immediately published and passed to the application on arrival. As data is temporarily stored inside ndn-svs anyway, the latter aspect should not have negative downsides from my understanding. We can of course discuss this aspect, though, and could add the queue back in, if needed.
  • Consumers and Producers are now not intended to be used by a user anymore and are only used internally (and are constructed based on the application graph and a node name). The registration of consumer callbacks and pushing data are now performed by providing the respective edge name.
  • The IceflowScaler class now accepts an IceFlow object instead of individual producers and consumers
  • Besides a consumer callback, users can also register a "prosumer" callback that itself can access an interface for pushing data to downstream consumers.

Looking forward to your feedback – if you like, I can also walk you through the PR on Monday.

@JKRhb
Copy link
Contributor Author

JKRhb commented Sep 13, 2024

Applied some further adjustments to the prosumer callback registration, that allows the user to specify which downstream edge they to push to, in order to make to it possible to push data to multiple downstream edges at once.

Copy link
Contributor Author

@JKRhb JKRhb left a comment

Choose a reason for hiding this comment

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

Noticed a few more spots that might require discussion below.

include/iceflow/producer.hpp Outdated Show resolved Hide resolved
@JKRhb JKRhb force-pushed the iceflow-dag-api branch 2 times, most recently from b6bba91 to 7be7684 Compare September 17, 2024 10:50
@JKRhb
Copy link
Contributor Author

JKRhb commented Sep 17, 2024

I think after the recent commits, this PR should now be ready for (hopefully) a final review :)

@JKRhb
Copy link
Contributor Author

JKRhb commented Sep 17, 2024

(Just spend about 15 minutes debugging some newly appearing errors involving segmentation faults – in the end, the fix came down to a6c21c5 🤦 This is always great fun :D)

@JKRhb
Copy link
Contributor Author

JKRhb commented Sep 17, 2024

When the pipeline succeeds, I am now going to finally squash and merge this PR :)

@JKRhb JKRhb enabled auto-merge September 17, 2024 15:59
@JKRhb JKRhb merged commit f6f8544 into main Sep 17, 2024
2 checks passed
@JKRhb JKRhb deleted the iceflow-dag-api branch September 17, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants