reef icon indicating copy to clipboard operation
reef copied to clipboard

[REEF-2054] Elastic group communication: broadcast

Open interesaaat opened this issue 7 years ago • 6 comments

This PR introduces the new elastic group communication framework. For the moment only the broadcast operator with flat topology is implemented. Further operators and topologies will be added in successive PRs.

This PR uses some of the pieces of PR #1479 so the latter should be merged before merging the former.

JIRA: REEF-2054 Elastic Broadcast

interesaaat avatar Jan 05 '19 00:01 interesaaat

Ready for the second round!

interesaaat avatar Jan 15 '19 21:01 interesaaat

Few very minor comments for today. Check for naming conventions and make

sure you use {0}-style string interpolation in logging instead of string concatenation or $"". (Note that for exceptions it's the other way around)

Can you please explain to me the intuition behind this?

Sure! In case of explicit string concatenation or $"..." - style interpolation, the entire string is evaluated before the method invocation. This is exactly what we need for exceptions and most of other methods and constructors as well.

For logging, the situation is different, as we don't know if the string will be used at all. If the log level is higher than the priority of the current log message, the logging call is basically a no-op, and the string parameter will be ignored. That is, if we build a string before the logging call, it can be a big waste, especially if the log message is long and expensive to construct. This is why for logging we want to perform string interpolation inside the logging method, so the logger can do it only when it knows for sure that we need to log that string. Sometimes we even wrap the logging call into Logger.IsLoggable() check, if interpolation at the logger level is not possible.

Internal logging string interpolation uses {0}-style placeholders, hence the rule: use {0} style for logging, $"..." for everything else.

Hope that helps! :)

Cheers, Sergiy.

motus avatar Jan 31 '19 01:01 motus

Here's another round of comments. Also, please remove the trailing spaces that popped up in the last commit 55aedfa

Sorry @motus which trailing spaces are you referring to?

interesaaat avatar Feb 14 '19 18:02 interesaaat

Let me take track of the major changes I will have to do (once you are done with the "syntactic" pass). I will update this list as we agree on the changes.

  • remove the id - 1, +1 thing. Move everything to 0-indexed;
  • configurations are immutable: fix the methods passing ref configurations
  • change custom retry logic with Microsoft.Practices.EnterpriseLibrary.TransientFaultHandling
  • discuss on incompatibility between DefaultFailureStates and IFailureState. Should we remove enum or remove IFailureState?

interesaaat avatar Feb 18 '19 18:02 interesaaat

@motus I am checking the protobuf thing and is not that easy to do. I mean, I can probably change the serialization/deserialization of messages to use protobuff, but to completely use protobuff end to end I will have to go and change the network service (which is not my code) and this may require some indefinite amount of time. Do you think it is ok if I only try protobuff for my messages?

interesaaat avatar Jul 17 '19 00:07 interesaaat

Hi Matteo,

Glad that you are back to REEF! Thanks for looking at it. I meant only the messages that you create and serialize on your own, not all the Wake stuff. I am on vacation toll Wednesday - let's discuss it next week when I am back!

Thank you, Sergiy.

On Tue, Jul 16, 2019 at 5:22 PM Matteo Interlandi [email protected] wrote:

@motus https://github.com/motus I am checking the protobuf thing and is not that easy to do. I mean, I can probably change the serialization/deserialization of messages to use protobuff, but to completely use protobuff end to end I will have to go and change the network service (which is not my code) and this may require some indefinite amount of time. Do you think it is ok if I only try protobuff for my messages?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apache/reef/pull/1487?email_source=notifications&email_token=AAAHVFCQMSEEVFNR24PNZBTP7ZQ43A5CNFSM4GNZUE7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2CUH2Q#issuecomment-512050154, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAHVFHM7ER6NYM4KTXP2A3P7ZQ43ANCNFSM4GNZUE7A .

motus avatar Jul 19 '19 17:07 motus