Skip to content

fix #947 and #1004 Add getContext() to Signal from materialize/doOnEach#1002

Merged
simonbasle merged 2 commits intomasterfrom
947-signalHasContext
Jan 5, 2018
Merged

fix #947 and #1004 Add getContext() to Signal from materialize/doOnEach#1002
simonbasle merged 2 commits intomasterfrom
947-signalHasContext

Conversation

@simonbasle
Copy link
Copy Markdown
Contributor

@simonbasle simonbasle commented Jan 3, 2018

  • filled for doOnEach
  • not filled for materialize for now

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 3, 2018 •
edited
Loading

Codecov Report

Merging #1002 into master will increase coverage by 0.13%.
The diff coverage is 88.63%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1002      +/-   ##
============================================
+ Coverage     83.59%   83.72%   +0.13%     
- Complexity     3388     3402      +14     
============================================
  Files           328      329       +1     
  Lines         26744    26763      +19     
  Branches       4930     4938       +8     
============================================
+ Hits          22356    22408      +52     
+ Misses         2893     2881      -12     
+ Partials       1495     1474      -21
Impacted Files Coverage Δ Complexity Δ
...ore/src/main/java/reactor/core/publisher/Flux.java 100% <ø> (ø) 481 <0> (ø) ⬇️
...main/java/reactor/core/publisher/MonoDoOnEach.java 100% <100%> (ø) 2 <2> (?)
...n/java/reactor/core/publisher/ImmutableSignal.java 84.78% <100%> (+5.71%) 23 <2> (+2) ⬆️
...ore/src/main/java/reactor/core/publisher/Mono.java 96.64% <100%> (-0.03%) 237 <0> (-3)
...n/java/reactor/core/publisher/FluxMaterialize.java 96.96% <100%> (+0.09%) 2 <0> (ø) ⬇️
...ain/java/reactor/core/publisher/MonoCacheTime.java 84.07% <50%> (+1.03%) 10 <0> (ø) ⬇️
...main/java/reactor/core/publisher/FluxDoOnEach.java 77.27% <72.72%> (+1.46%) 2 <0> (ø) ⬇️
...e/src/main/java/reactor/core/publisher/Signal.java 85.71% <91.66%> (ø) 31 <14> (+6) ⬆️
...rc/main/java/reactor/core/publisher/Operators.java 72.15% <0%> (+0.36%) 84% <0%> (+1%) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9e3aa9...276ff8a. Read the comment docs.

@simonbasle simonbasle mentioned this pull request Jan 3, 2018
Closed
@smaldini
Copy link
Copy Markdown
Contributor

smaldini commented Jan 3, 2018

Use currentContext vs getContext -
Eventually cache currentContext in FluxOnEach.

@dfeist
Copy link
Copy Markdown
Contributor

dfeist commented Jan 3, 2018

@simonbasle @smaldini In my custom implementation for this as shown in the github issue, not caching the context produced a not insignificant overhead. Unless it's particularly hard to cache this, I'd try to include that in this PR.

@simonbasle simonbasle force-pushed the 947-signalHasContext branch from c71e44c to 16e6fd2 Compare January 4, 2018 15:50
@simonbasle simonbasle changed the title fix #947 add nullable getContext() to Signal fix #947 and #1004 Add getContext() to Signal from materialize/doOnEach Jan 4, 2018
@simonbasle simonbasle requested a review from smaldini January 4, 2018 16:02
@simonbasle simonbasle self-assigned this Jan 4, 2018
@simonbasle simonbasle added the type/enhancement A general enhancement label Jan 4, 2018
@simonbasle simonbasle added this to the 3.1.3.RELEASE milestone Jan 4, 2018
@simonbasle
Copy link
Copy Markdown
Contributor Author

simonbasle commented Jan 4, 2018

@smaldini regarding the naming, I kept it consistent with other methods in Signal (notably getType()). The subscriber would both implement getContext() and currentContext(), of course...
Added commit to align materialize with this change and cache the Context in both cases, as @dfeist suggested there was a significant benefit in doing so.

smaldini
smaldini reviewed Jan 4, 2018
View reviewed changes
reactor-core/src/main/java/reactor/core/publisher/ImmutableSignal.java Outdated

private static final long serialVersionUID = -2004454746525418508L;

private final Context context;
Copy link
Copy Markdown
Contributor

@smaldini smaldini Jan 4, 2018

Choose a reason for hiding this comment

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

maybe transient, context is not serializable right now (should it?)

@simonbasle simonbasle force-pushed the 947-signalHasContext branch from 2116aaa to 41d6854 Compare January 4, 2018 16:52
@smaldini smaldini mentioned this pull request Jan 4, 2018
Closed
dfeist
dfeist approved these changes Jan 4, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@dfeist dfeist left a comment

Choose a reason for hiding this comment

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

Looks good!

@simonbasle
fix #947 Add getContext() to Signal
ddc7d6c 
 - filled for doOnEach
 - not filled for materialize for now
 - defaults to Context.empty() when not filled
 - use singleton if Signal.complete() built with empty Context
@simonbasle
fix #1004 Materialize Signals also have Context, which are cached
276ff8a 
This commit adds the Context to Signals emitted by materialize.
Additionally, both in materialize and in doOnEach the Context is
cached, only fetched from `actual` upon subscriber construction.

Additionally the ImmutableSignal Context is transient, since Context
is not Serializable.

Make Signal.context field transient (Context is not Serializable)
@simonbasle simonbasle force-pushed the 947-signalHasContext branch from 80a3735 to 276ff8a Compare January 5, 2018 09:39
@simonbasle simonbasle merged commit b08ba93 into master Jan 5, 2018
@simonbasle simonbasle deleted the 947-signalHasContext branch January 5, 2018 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@smaldini smaldini smaldini approved these changes

@dfeist dfeist dfeist approved these changes

@OlegDokuka OlegDokuka OlegDokuka approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@simonbasle simonbasle

Labels

type/enhancement A general enhancement

Projects

None yet

Milestone

3.1.3.RELEASE

Development

Successfully merging this pull request may close these issues.

5 participants

@simonbasle @codecov-io @smaldini @dfeist @OlegDokuka