search

akka / akka-http

The Streaming-first HTTP server/module of Akka
https://doc.akka.io/docs/akka-http
Other
1.34k stars 594 forks source link

PrepareMessages in WebSocket code can be made a GraphStage or splitAfter #62

Open akka-ci opened 8 years ago

akka-ci commented 8 years ago

Issue by ktoso Thursday Mar 24, 2016 at 19:08 GMT Originally opened as https://github.com/akka/akka/issues/20136

We had it implemented using splitWhen, but we could actually implement it as graph stage or splitAfter, to be decided which way is cleaner - perhaps the graph stage actually - as it could collapse many steps into one?

See here: https://github.com/akka/akka/pull/20089/files/864f342f4c0da02420225259d154e7c293b71f6e#diff-d7697d316b990df80d5e2d4308e7b93fR119

akka-ci commented 8 years ago

Comment by viktorklang Wednesday Mar 30, 2016 at 10:56 GMT

In order to let others have a chance at implementing this, how about describing briefly what it means?

akka-ci commented 8 years ago

Comment by ktoso Wednesday Mar 30, 2016 at 10:58 GMT

I don't see what you mean. It describes what is to be done with pointing to the point in the code even: https://github.com/akka/akka/pull/20089/files#diff-d7697d316b990df80d5e2d4308e7b93fR126

Take that flow and make it a GraphStage instead of multiple splits

akka-ci commented 8 years ago

Comment by viktorklang Wednesday Mar 30, 2016 at 11:13 GMT

Then I can only conclude that I am completely daft. Because that comment does not instruct me what is to be done, simply replace splitWhen with splitAfter? (I'd think not)

akka-ci commented 8 years ago

Comment by ktoso Wednesday Mar 30, 2016 at 11:18 GMT

It is explained in my first sentence of this ticket, and I repeated the same thing just now – "Take that flow and make it a GraphStage instead of multiple splits", what's missing in that definition?

akka-ci commented 8 years ago

Comment by viktorklang Wednesday Mar 30, 2016 at 11:26 GMT

There is no multiple splits, I see only 1?

akka-ci commented 8 years ago

Comment by ktoso Wednesday Mar 30, 2016 at 11:33 GMT

C'mon Viktor, please don't make this a dragged out catching-everyone-by-each-word discussion now.

The following piece of code, dancing around the splitting and collecting can likely be simplified, it's just about that:

     /* Collects user-level API messages from MessageDataParts */
val collectMessage: Flow[MessageDataPart, Message, NotUsed] =

     def prepareMessages: Flow[MessagePart, Message, NotUsed] =
Flow[MessagePart]
.via(PrepareForUserHandler)
.splitWhen(_.isMessageEnd) // FIXME using splitAfter from #16885 would simplify protocol a lot
.collect {
case m: MessageDataPart ⇒ m
}
.via(collectMessage)
.concatSubstreams
.named("ws-prepare-messages")
akka-ci commented 8 years ago

Comment by viktorklang Wednesday Mar 30, 2016 at 11:38 GMT

Konrad, I'm sorry, it wasn't immediately clear to me what was to be done. I had 2 options: A) not ask B) ask

I chose B.

akka-ci commented 8 years ago

Comment by ktoso Wednesday Mar 30, 2016 at 11:55 GMT

Which is fine and great, I really didn't see what's missing and the explanation seemed being ignored – too much javadsl work leaves me even more nervous than usually, sorry if overreacted.

Goal of ticket should be clear now I hope :)

akka-ci commented 8 years ago

Comment by viktorklang Wednesday Mar 30, 2016 at 12:07 GMT

Np, I'm not 100% sure if I know everything which needs to be changed but I now at least have some idea :)

On Wed, Mar 30, 2016 at 1:56 PM, Konrad Malawski notifications@github.com wrote:

Which is fine and great, I really didn't see what's missing and the explanation seemed being ignored – too much javadsl work leaves me even more nervous than usually, sorry if overreacted.

Goal of ticket should be clear now I hope :)

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/akka/akka/issues/20136#issuecomment-203396057

Cheers, √