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

Fix mapConcat context propagation #30289

Merged
merged 4 commits into from
Jun 8, 2021
Merged

Fix mapConcat context propagation #30289

merged 4 commits into from
Jun 8, 2021

Conversation

ygree
Copy link
Contributor

@ygree ygree commented Jun 7, 2021

Fix mapConcat context propagation, so consecutive child elements are connected to the original span when using Telemetry tracing

@ygree ygree self-assigned this Jun 7, 2021
@akka-ci akka-ci added the validating PR is currently being validated by Jenkins label Jun 7, 2021
@patriknw patriknw added the 2 - pick next Used to mark issues which are next up in the queue to be worked on. The tag is non-binding label Jun 7, 2021
jrudolph
jrudolph reviewed Jun 7, 2021
View reviewed changes
akka-stream/src/main/scala/akka/stream/impl/fusing/Ops.scala Outdated
} catch handleException

override def onUpstreamFinish(): Unit = onFinish()

override def onPull(): Unit =
try pushPull()
try pushPull(pullFirstSubElement = true)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be pullFirstSubElement = false here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented a test on Telemetry side to catch this issue.

jrudolph
jrudolph reviewed Jun 7, 2021
View reviewed changes
Copy link
Member

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

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

Even if I know what the code should do, I find it hard to compare the code with the expectation of what it should do.

Could the conditions be removed (for a bit of performance hit)?

Then we could always suspendContext after an element was grabbed, and always resume/suspend after an element was pushed.

@ygree
Copy link
Contributor Author

ygree commented Jun 7, 2021 •
edited
Loading

Even if I know what the code should do, I find it hard to compare the code with the expectation of what it should do.

Could the conditions be removed (for a bit of performance hit)?

Then we could always suspendContext after an element was grabbed, and always resume/suspend after an element was pushed.

It would require an additional suspend / resume pair for each first element. That's why I introduced this condition.
We can have two version of pushPull to remove this param. WDYT?

@akka-ci akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Jun 7, 2021
@akka-ci
Copy link

akka-ci commented Jun 7, 2021

Test PASSed.

1 similar comment
@akka-ci
Copy link

akka-ci commented Jun 7, 2021

Test PASSed.

raboof
raboof approved these changes Jun 7, 2021
View reviewed changes
Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Seems relatively low-risk from the core Akka perspective, but indeed not super easy to follow. If that is well-tested on the Telemetry side then that's perhaps fine?

@ygree ygree mentioned this pull request Jun 8, 2021
Closed
@ygree
Copy link
Contributor Author

ygree commented Jun 8, 2021

@jrudolph This is an alternative implementation to reduce switch logic: #30290

@akka-ci akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Jun 8, 2021
@ygree ygree requested a review from jrudolph June 8, 2021 09:07
jrudolph
jrudolph approved these changes Jun 8, 2021
View reviewed changes
Copy link
Member

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

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

LGTM

@akka-ci akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Jun 8, 2021
@akka-ci
Copy link

akka-ci commented Jun 8, 2021

Test PASSed.

@raboof raboof merged commit 3da5c2b into akka:master Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrudolph jrudolph jrudolph approved these changes

@raboof raboof raboof approved these changes

Assignees

@ygree ygree

Labels
2 - pick next Used to mark issues which are next up in the queue to be worked on. The tag is non-binding t:stream tested PR that was successfully built and tested by Jenkins
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ygree @akka-ci @jrudolph @raboof @patriknw