Skip to content

Issue/686 part 2 - #764

Closed
bnordli wants to merge 4 commits into
dolittle:masterfrom
bnordli:issue/686_part_2
Closed

Issue/686 part 2#764
bnordli wants to merge 4 commits into
dolittle:masterfrom
bnordli:issue/686_part_2

Conversation

@bnordli

@bnordli bnordli commented Jan 20, 2017

Copy link
Copy Markdown
Contributor

Continuing on #686

…olittle#686

Exceptions from the following calls will be forwarded to anything implementing IExceptionSubscriber
- queries
- commands
- sagas
- calls to rest handlers

Original issue: ProCoSys#9
From the ProCoSys fork.

Instead of depending on IInstancesOf<T>, any implementation can depend on IOrderedInstancesOf<T> instead.

By doing this, the ordering of Ts are guaranteed: Implementations of T can specify their ordering, either explicitly by and Order attribute, or implicitly via an After attribute.
…olittle#686

Exceptions from the following calls will be forwarded to anything implementing IExceptionSubscriber
- queries
- commands
- sagas
- calls to rest handlers

Original issue: ProCoSys#9

# Conflicts:
#	Source/Bifrost/Bifrost.csproj
@einari

einari commented Jan 20, 2017

Copy link
Copy Markdown
Contributor

Could this be split into 3? The CommandHandlerInvoker part (42be6f0) is the least intrusive.

The OrderedInstancesOf part I'd like to discuss. Need to understand the need and the implications, as it feels a bit "un-bifrosty" with regards to it creating couplings - and attributes are something we've been trying to avoid for that reason.

The IExceptionSubscriber is very interesting and in line with thoughts I've been having and I want to pursue that one for general diagnostics purposes and maybe take it in a slightly different direction.

@bnordli

bnordli commented Jan 20, 2017

Copy link
Copy Markdown
Contributor Author
  1. CommandHandlerInvoker moved to a separate PR.

  2. What do you mean about the IExceptionSubscriber? Do you want me to make a separate PR of that as well, or do you instead want to implement it differently?

  3. About IOrderedInstancesOf, yes I know this may add some couplings, but it solves some real world problems(ensuring the order of injected instances) and reduces couplings elsewhere. An adapted example from the real world coming up.

@bnordli

bnordli commented Jan 20, 2017

Copy link
Copy Markdown
Contributor Author

..say you have a processor that takes an object and sends it to multiple other instances (very common scenario). If the order is not important, this class is simply

public class Processor : IProcessor
{
  IList<ICanProcess> canProcess;

  public Processor(IInstancesOf<ICanProcess> canProcess)
  {
    this.canProcess = canProcess.ToList();
  }
  
  public Process(SomeObject obj) => canProcess.ForEach(cp => cp.Process(obj));
}

However, if there are dependencies between any of these objects (for any reason, believe me, we had many examples of this in the ProCoSys code base), the simplest implementation is:

public class Processor : IProcessor
{
  IList<ICanProcess> canProcess;

  public Processor(IFirstProcessor first, ISecondProcessor second, IDontCareProcessor any)
  {
    this.canProcess = new ICanProcess[] {
      first,
      second,
      any,
    };
  }
  
  public Process(SomeObject obj) => canProcess.ForEach(cp => cp.Process(obj));
}

..leading to one interface per processor, and strong coupling between Processor and these interfaces. Any addition or removal of processors leads to a change in this class. (And in the ProCoSys Import project we had classes forwarding to 10-15 other classes like this.)

So I propose IOrderedInstancesOf to solve this problem, reducing Processor again to

public class Processor : IProcessor
{
  IList<ICanProcess> canProcess;

  public Processor(IOrderedInstancesOf<ICanProcess> canProcess)
  {
    this.canProcess = canProcess.ToList();
  }
  
  public Process(SomeObject obj) => canProcess.ForEach(cp => cp.Process(obj));
}

with the only other change to either

  1. Add [Order(-1)] to FirstProcessor (now only implementing ICanProcess), or
  2. add [After(typeof(FirstProcessor))] to SecondProcessor (also only implementing ICanProcess).

And the cost is that in any case, one of these must reference Bifrost.Execution, and in the second SecondProcessor is coupled to FirstProcessor. (I deliberately did not implement Before, to make sure coupling only go in the "correct" direction, chronologically.)

I think this cost is in many cases justified.

@einari

einari commented Jan 20, 2017

Copy link
Copy Markdown
Contributor

Great. Thanks, lets keep the IOrderedInstances and IExceptionSubscriber thing here for now. I will put these on the agenda for the community standup on Monday to be discussed. Easier to understand the requirement I think.

This was referenced Jan 25, 2017
@bnordli

bnordli commented Jan 25, 2017

Copy link
Copy Markdown
Contributor Author

Broken out into #773 and #774.

@bnordli bnordli closed this Jan 25, 2017
@bnordli
bnordli deleted the issue/686_part_2 branch January 25, 2017 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants