Allow complex filters for ImplicitStreamSubscriptionAttribute #2988

DixonDs · 2017-05-01T13:00:19Z

My attempt to address #1747

jason-bragg · 2017-05-01T22:04:52Z

src/Orleans/Streams/PubSub/ImplicitStreamSubscriberTable.cs

+            if (!table.TryGetValue(streamNamespace, out entry))
+            {
+                entry = FindImplicitSubscribers(streamNamespace);
+                table[streamNamespace] = entry;


Since table is built at runtime, rather than at start, it needs to be a concurrent dictionary, correct? Or is their some other concurrency guarantee that I'm missing?

@jason-bragg Yes, you are right. There was ConcurrentDictionary initially, but then we dealing with some serializer codegen issue I reverted it back to Dictionary and forgot to change it back

jason-bragg · 2017-05-01T23:23:39Z

src/Orleans/Streams/Predicates/IStreamNamespacePredicate.cs

+{
+    public interface IStreamNamespacePredicate
+    {
+        bool IsMatch(string streamNamespace);


If we're going to support custom filtering, I suggest that we widen the filter to include stream full stream id, that is stream guid, namespace, and provider name. Something like:

bool IsMatch(string streamProviderName, IStreamIdentity streamIdentity);

@jason-bragg The only problem with this that it limits the cachebility of the subscribers in ImplicitStreamSubscriberTable since you have to recalculate that list per stream ID + namespace + provider name. With stream IDs having dynamic nature, I guess, it might have significant impact.

What do you think the performance penalty will be in this case? If it is insignificant, I will be happy to make this change.

I think we should be careful here to introduce filtering on streamId into implicit subscribe. Aside from the performance penalty DixonDs brought up, it conflicts with the nature of explicit subscribe too. Because of the dynamic nature of streamId, filtering/subscribe to a stream fits more naturally into the territory of explicit subscribe. Adding the same feature into implicit subscribe may bring more confusion to users , potentially bugs in edge cases. I think we should defer this feature until specifically asked by users

jason-bragg · 2017-05-01T23:31:05Z

src/Orleans/Core/GrainAttributes.cs

@@ -346,18 +347,26 @@ public LogConsistencyProviderAttribute()
                ProviderName = Runtime.Constants.DEFAULT_LOG_CONSISTENCY_PROVIDER_NAME;
            }
        }
-
    }

    [AttributeUsage(AttributeTargets.Class, AllowMultiple = true)]
    public sealed class ImplicitStreamSubscriptionAttribute : Attribute


Suggest introduction of
abstract class ImplicitStreamSubscriptionBaseAttribute where TPredicate : IStreamNamespacePredicate. new()

with ImplicitStreamSubscriptionAttribute : ImplicitStreamSubscriptionBaseAttribute

So developers can introduce their own attributes with arguments (which simply providing a type does not allow). For instance
RegExImplicitStreamSubscriptionAttribute : ImplicitStreamSubscriptionBaseAttribute with constructors that take a RegEx expression.

@jason-bragg I've added another ctor to ImplicitStreamSubscriptionAttribute and removed sealed to allow this kind of extensibility. I didn't go with the abstract base class you suggested, because 1) attributes cannot be generic thus you cannot put generic restrictions on the predicate type like where TPredicate : IStreamNamespacePredicate, new() 2) having a non-abstract attribute with a ctor accepting the predicate type seems to be useful on its own (see FilteredImplicitSubscriptionGrain in tests) so that you don't have to implement your own attribute just to pass the predicate type.

So in the end, it looked like there was no extra benefit of that base attribute type.

jason-bragg · 2017-05-01T23:38:50Z

src/Orleans/Streams/PubSub/ImplicitStreamSubscriberTable.cs

@@ -9,10 +10,12 @@ namespace Orleans.Streams
    internal class ImplicitStreamSubscriberTable
    {
        private readonly Dictionary<string, HashSet<int>> table;
+        private readonly List<Tuple<IStreamNamespacePredicate, int>> predicates;


This class needs be serializable. I don't think this change takes that into account.

@jason-bragg As far I understand, it should work as long as IStreamNamespacePredicate implementations are serializable. At least it is what I've seen when debugging the tests.

xiazen · 2017-05-02T18:01:41Z

src/Orleans/Streams/PubSub/ImplicitStreamSubscriberTable.cs

            }

-            var attribs = grainClass.GetTypeInfo().GetCustomAttributes<ImplicitStreamSubscriptionAttribute>(inherit: false);
+            var attribs = grainClass.GetTypeInfo().GetCustomAttributes<ImplicitStreamSubscriptionAttribute>(false);


I think this should be GetCustomAttributes(true) ? instead of false? since user can inherit ImplicitStreamSubscriptionAttribute now?

@xiazen Correct me if I am wrong but inherit: true doesn't have anything to do with the attribute inheritance, but rather the target type inheritance marked by the attribute. GetCustomAttributes<ImplicitStreamSubscriptionAttribute> returns not only ImplicitStreamSubscriptionAttribute but all its descendants - from docs: "The type of attribute to search for. Only attributes that are assignable to this type are returned."

I think you are correct. I misread the documentation as

inherit Type: System.Boolean true to inspect the descendants of element; otherwise, false.

The correct doc is true to inspect the ancestors of element; otherwise, false.

xiazen · 2017-05-02T18:07:04Z

src/Orleans/Streams/PubSub/ImplicitStreamSubscriberTable.cs

-        private void AddImplicitSubscriber(Type grainClass, ISet<string> namespaces)
+        /// <param name="streamNamespace">The stream namespace to find subscribers too.</param>
+        /// <returns></returns>
+        private HashSet<int> FindImplicitSubscribers(string streamNamespace)


nit: This method is finding type code of implicit subscribers right? FindImplicitSubscriberTypeCode may be less confusing.

xiazen · 2017-05-02T18:07:49Z

src/Orleans/Streams/PubSub/ImplicitStreamSubscriberTable.cs

-            HashSet<int> entry;
-            return (table.TryGetValue(streamNamespace, out entry) && // if we don't have implictit subscriptions for this namespace, fail out
-                    entry.Contains(grainIdTypeCode));                 // if we don't have an implicit subscription for this type of grain on this namespace, fail out
+            HashSet<int> entry = GetOrAddImplicitSubscribersSet(streamNamespace);


same here: GetOrAddImplicitSubscriberTypeCodes will be a less confusing name

xiazen · 2017-05-02T18:08:40Z

src/Orleans/Streams/PubSub/ImplicitStreamSubscriberTable.cs

            return result;
        }

+        private HashSet<int> GetOrAddImplicitSubscribersSet(string streamNamespace)


nit: This method is get or add type codes of implicit subscribers right? GetOrAddImplicitSubscriberTypeCodes will be a less confusing name

xiazen · 2017-05-02T18:14:13Z

src/Orleans/Streams/Predicates/IStreamNamespacePredicate.cs

+{
+    public interface IStreamNamespacePredicate
+    {
+        bool IsMatch(string streamNamespace);


nit on naming : bool Match(string namespace). IsMatch is weird to me grammatically.

ignore this... I found out Regex is doing the same naming too. So IsMatch is good with me

xiazen · 2017-05-02T18:35:33Z

src/Orleans/Streams/Predicates/IStreamNamespacePredicate.cs

+{
+    public interface IStreamNamespacePredicate
+    {
+        bool IsMatch(string streamNamespace);


I think we should be careful here to introduce filtering on streamId into implicit subscribe. Aside from the performance penalty DixonDs brought up, it conflicts with the nature of explicit subscribe too. Because of the dynamic nature of streamId, filtering/subscribe to a stream fits more naturally into the territory of explicit subscribe. Adding the same feature into implicit subscribe may bring more confusion to users , potentially bugs in edge cases. I think we should defer this feature until specifically asked by users

xiazen · 2017-05-02T18:49:26Z

src/Orleans/Streams/Predicates/ExactMatchStreamNamespacePredicate.cs

+namespace Orleans.Streams
+{
+    [Serializable]
+    internal class ExactMatchStreamNamespacePredicate : IStreamNamespacePredicate


I think we should also have a RegexStreamNameSpacePredicate

public class RegexStreamNameSpacePredict { private readonly Regex regex; public RegexStreamNameSpacePredict( Regex regex) { this.regex = regex; } bool Match(string streamNameSpace) { return this.regex.IsMatch(streamNameSpace); } }

This will be a very common IStreamNamespacePredicate which many users will be interested in

@xiazen Shall I add also RegexImplicitStreamSubscriptionAttribute in this case since this predicate requires a parameter to be passed?

Yes I think you are correct! Although I'm not sure it is the correct name.
RegexStreamNameSpacePredict didn't change the nature of ImplicitSubscribe, which is subscribe through annotation on grain class. It just change the underlying mechanism of name space filtering.
But I cannot think of a better name. I guess I'm fine with the name. If you can come up with a better one then it is great!

xiazen · 2017-05-02T18:52:01Z

src/Orleans/Streams/Predicates/ExactMatchStreamNamespacePredicate.cs

+namespace Orleans.Streams
+{
+    [Serializable]
+    internal class ExactMatchStreamNamespacePredicate : IStreamNamespacePredicate


why is ExactMatchStreamNamespacePredicate internal? it should be public to external users right?

@xiazen It is used in ImplicitStreamSubscriptionAttribute, so I didn't see a point to expose it externally, since the users can just use ImplicitStreamSubscriptionAttribute with ctor accepting the stream namespace.

…onAttribute

xiazen · 2017-05-03T18:10:19Z

src/Orleans/Core/GrainAttributes.cs

@@ -374,4 +375,13 @@ public ImplicitStreamSubscriptionAttribute(IStreamNamespacePredicate predicate)
            Predicate = predicate;
        }
    }
+
+    [AttributeUsage(AttributeTargets.Class, AllowMultiple = true)]
+    public sealed class RegexImplicitStreamSubscriptionAttribute : ImplicitStreamSubscriptionAttribute


Add xml comment. And look through your build warnings to see if it asks you to fill in any other xml comment. Thanks ! Great work !

xiazen · 2017-05-03T18:19:54Z

src/Orleans/Streams/Predicates/RegexStreamNamespacePredicate.cs

+{
+    [Serializable]
+    public class RegexStreamNamespacePredicate : IStreamNamespacePredicate
+    {


Actually I found 0 warnings. But I think it still make sense to add some xml comment on the new predicate and RegexImplicitStreamSubscriptionAttribute . But other than that. I Approve!

Thanks for the work ! It is great

xiazen · 2017-05-03T18:20:28Z

@dotnet-bot test this please

DixonDs · 2017-05-08T21:45:29Z

@jason-bragg is it ready for merging?

jason-bragg · 2017-05-08T23:38:06Z

src/Orleans/Streams/PubSub/ImplicitStreamSubscriberTable.cs

@@ -8,11 +10,13 @@ namespace Orleans.Streams
    [Serializable]
    internal class ImplicitStreamSubscriberTable
    {
-        private readonly Dictionary<string, HashSet<int>> table;
+        private readonly ConcurrentDictionary<string, HashSet<int>> table;


Table is now just a cache, and is dynamically updated at runtime, it no longer should be serialized as part of the ImplicitStreamSubscriberTable.
The predicates are ultimately the authority and the only data that needs serialized.
Or am I missing something?

@jason-bragg I guess it depends on what is more efficient - to serialize the cache or to rebuild it on demand. I guess it depends on what predicates are there - since it could be the application code, it is a kind of a black box for us.

But I guess, normally, rebuilding the cache should be faster, so I've done the suggested change.

jason-bragg · 2017-05-08T23:41:29Z

Sorry for delayed review.. One last thing, then I think it's good.
Thanks a lot!

jason-bragg · 2017-05-09T16:12:36Z

Thanks @DixonDs
LG2M
Will merge once tests are complete, if @xiazen doesn't beat me to it :)

dnfclas added the cla-already-signed label May 1, 2017

Allow complex filters for ImplicitStreamSubscriptionAttribute

cd06f29

DixonDs force-pushed the implicit-subscription-filter branch from 7dd72e3 to cd06f29 Compare May 1, 2017 13:19

jason-bragg reviewed May 1, 2017

View reviewed changes

jason-bragg requested a review from xiazen May 1, 2017 23:39

Dmytro Dziuma added 2 commits May 2, 2017 13:02

Use ConcurrentDictionary in ImplicitStreamSubscriberTable

163d359

Allow extension of ImplicitStreamSubscriptionAttribute

7ded164

xiazen reviewed May 2, 2017

View reviewed changes

Dmytro Dziuma added 2 commits May 2, 2017 22:13

Methods names

260db86

Added RegexStreamNamespacePredicate and RegexImplicitStreamSubscripti…

30bddcf

…onAttribute

xiazen reviewed May 3, 2017

View reviewed changes

xiazen approved these changes May 3, 2017

View reviewed changes

Xml docs

5fb4aac

jason-bragg reviewed May 8, 2017

View reviewed changes

Don't serialize ImplicitStreamSubscriberTable.table field

12985f6

sergeybykov added this to the 1.5.0 milestone May 10, 2017

sergeybykov merged commit 29a24ed into dotnet:master May 10, 2017

sergeybykov mentioned this pull request May 10, 2017

ImplicitStreamSubscription via Regex(or other) #1747

Closed

DixonDs deleted the implicit-subscription-filter branch May 10, 2017 06:29

xiazen mentioned this pull request May 16, 2017

Pass stream namespace to implicit subscription grain #2987

Closed

github-actions bot locked and limited conversation to collaborators Dec 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow complex filters for ImplicitStreamSubscriptionAttribute #2988

Allow complex filters for ImplicitStreamSubscriptionAttribute #2988

DixonDs commented May 1, 2017

jason-bragg May 1, 2017

DixonDs May 2, 2017

jason-bragg May 1, 2017

DixonDs May 2, 2017

xiazen May 2, 2017

jason-bragg May 1, 2017

DixonDs May 2, 2017

jason-bragg May 1, 2017

DixonDs May 2, 2017

xiazen May 2, 2017

DixonDs May 2, 2017

xiazen May 2, 2017

xiazen May 2, 2017

xiazen May 2, 2017

xiazen May 2, 2017

xiazen May 2, 2017

xiazen May 2, 2017

xiazen May 2, 2017

xiazen May 2, 2017 •

edited

Loading

DixonDs May 2, 2017

xiazen May 2, 2017

xiazen May 2, 2017

DixonDs May 2, 2017

xiazen May 3, 2017 •

edited

Loading

xiazen May 3, 2017

xiazen commented May 3, 2017

DixonDs commented May 8, 2017

jason-bragg May 8, 2017

DixonDs May 9, 2017

jason-bragg commented May 8, 2017

jason-bragg commented May 9, 2017

Allow complex filters for ImplicitStreamSubscriptionAttribute #2988

Allow complex filters for ImplicitStreamSubscriptionAttribute #2988

Conversation

DixonDs commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiazen May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiazen May 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiazen commented May 3, 2017

DixonDs commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jason-bragg commented May 8, 2017

jason-bragg commented May 9, 2017

xiazen May 2, 2017 •

edited

Loading

xiazen May 3, 2017 •

edited

Loading