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

Improve type safety for custom graph stages #3237

Merged
merged 12 commits into from
Jan 16, 2018

Conversation

marcpiechura
Copy link
Contributor

In order to make the API consistent I've used Inlet<T> and Outlet<T> on all public methods, even if they doesn't really need it, e.g. IsAvailable .
For those methods which are used internally and where no type argument could be provided, I've created a private version without type argument, e.g. Pull.

Since this is quite a breaking change we need to decide if we wait for the next major version or release it with the next minor since it would prevent some ugly runtime exceptions.

closes #3231


public override BidiShape<TIn1, TOut1, TIn2, TOut2> Shape { get; }
public override BidiShape<TIn, TIn, TOut, TOut> Shape { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure, this is right? It looks like less generic version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, the previous more generic version was actually a „bug“, only prevented from happening due to the fact that the public api already restricts it to two different types.
I got a compilation error at the following line
„Push(killSwitch.Out2, Grab(killSwitch.In2)),“
which makes it obvious that both, the inlets and outlets, have to be of the same type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type names are a bit confusing, T1 instead of TIn and T2 instead of TOut would make more sense, so I’ll update them when if fix the other issue with the performance test project.

@@ -50,7 +50,7 @@ public static class KillSwitches
/// <typeparam name="TOut1">TBD</typeparam>
/// <returns>TBD</returns>
public static IGraph<BidiShape<TIn1, TIn1, TOut1, TOut1>, UniqueKillSwitch> SingleBidi<TIn1, TOut1>
() => UniqueBidiKillSwitchStage<TIn1, TIn1, TOut1, TOut1>.Instance;
() => UniqueBidiKillSwitchStage<TIn1, TOut1>.Instance;
Copy link
Contributor Author

@marcpiechura marcpiechura Dec 31, 2017

Choose a reason for hiding this comment

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

This is the public api I’m referring to in the other comment

@Aaronontheweb
Copy link
Member

@marcpiechura looks like we have some merge conflicts with the API approval

@marcpiechura
Copy link
Contributor Author

fixed the conflicts

@Aaronontheweb
Copy link
Member

@marcpiechura

NBench.NBenchException: Error occurred during $Akka.Streams.Tests.Performance.InterpreterBenchmark+Graph_interpreter_100k_elements_with_1_identity RUN. ---> System.Reflection.TargetException: Non-static field requires a target.
   at System.Reflection.RtFieldInfo.CheckConsistency(Object target)
   at System.Reflection.RtFieldInfo.InternalSetValue(Object obj, Object value, BindingFlags invokeAttr, Binder binder, CultureInfo culture, StackCrawlMark& stackMark)
   at System.Reflection.RtFieldInfo.SetValue(Object obj, Object value, BindingFlags invokeAttr, Binder binder, CultureInfo culture)
   at Akka.Streams.Tests.Performance.InterpreterBenchmark.GraphDataSource`1..ctor(String toString, T[] data)
   at Akka.Streams.Tests.Performance.InterpreterBenchmark.<>c__DisplayClass4_0.<Execute>b__0(TestSetup setup, Func`1 lastEvents)
   at NBench.Sdk.Benchmark.WarmUp()
   --- End of inner exception stack trace ---

@Aaronontheweb
Copy link
Member

Error was from Performance.InterpreterBenchmark+Graph_interpreter_100k_elements_with_1_identity

@Horusiath Horusiath merged commit fd15e7b into akkadotnet:dev Jan 16, 2018
pzartem pushed a commit to pzartem/Akka.Streams.Kafka that referenced this pull request Mar 18, 2018
Updating dependencies: Akka.Streams 1.3.5, Confluent.Kafka 0.11.3,
MessagePack 1.7.3.4.
Also updating stages to support type-safe Akka.Streams API changes akkadotnet/akka.net/pull/3237
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make writing custom stream stages more type safe
3 participants