Skip to content

Conversation

svezfaz
Copy link
Contributor

@svezfaz svezfaz commented Feb 2, 2018

Refs #23895

@akka-ci
Copy link

akka-ci commented Feb 2, 2018

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

@ktoso
Copy link
Contributor

ktoso commented Feb 3, 2018

OK TO TEST

@ktoso
Copy link
Contributor

ktoso commented Feb 3, 2018

PLS WHITELIST

@akka-ci akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Feb 3, 2018
@akka-ci
Copy link

akka-ci commented Feb 3, 2018

Test FAILed.

@svezfaz svezfaz force-pushed the 23895-prematerialize branch from a62f2ca to 738f156 Compare February 3, 2018 11:18
@svezfaz
Copy link
Contributor Author

svezfaz commented Feb 3, 2018

Missed an import in the docs test. Should be passing now.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Feb 3, 2018
@akka-ci
Copy link

akka-ci commented Feb 3, 2018

Test PASSed.

Copy link
Contributor

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM with some minor comments

public void sourcePreMaterialization() {
//#source-prematerialization
Source<String, ActorRef> matValuePoweredSource =
Source.actorRef(Integer.MAX_VALUE, OverflowStrategy.fail());
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite agree Integer.MAX_VALUE is a reasonable buffer size ;)

Also, funky indentation here (and the lines below)

probe1.expectError().getMessage should ===("boom")
probe2.expectError().getMessage should ===("boom")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add one test case for when materialization of the matVal powered source fails also (.mapMaterializedValue(_ => throw booom)), mostly to document what happens.

@svezfaz svezfaz force-pushed the 23895-prematerialize branch from 738f156 to a407b9d Compare February 14, 2018 18:14
@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Feb 14, 2018
@akka-ci
Copy link

akka-ci commented Feb 14, 2018

Test PASSed.

* that can be used to consume elements from the newly materialized Source.
*/
def preMaterialize()(implicit materializer: Materializer): (Mat, ReprMat[Out, NotUsed]) = {
val (mat, pub) = toMat(Sink.asPublisher(fanout = true))(Keep.both).run()
Copy link
Contributor

Choose a reason for hiding this comment

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

good, let's keep it like that. Technically the end-user may want to decide that but let's not expose this decision until we actually go into implementing the real-thing, as then perhaps that would not be needed at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants