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

Getting a list of active grains in Orleans for monitoring #1772

Merged
merged 10 commits into from
May 25, 2016
Merged

Getting a list of active grains in Orleans for monitoring #1772

merged 10 commits into from
May 25, 2016

Conversation

Dewaldf
Copy link
Contributor

@Dewaldf Dewaldf commented May 17, 2016

Gives the developer to get a list of active grains from Orleans, for monitoring etc. See #1751.

Since the list that could be returned could be a very big list, I added an attribute that the developer will need to decorate his class with, if he wants that type of grain to be returned. This gives developers the opportunity to specify only certain grains.

…ics(SiloAddress[] hostsIds), GetDetailedGrainStatistics()) , that can be used to get more details about all the active grains.
…nt to be returned when GetDetailedGrainStatistics is called.
@dnfclas
Copy link

dnfclas commented May 17, 2016

Hi @Dewaldf, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

@dnfclas
Copy link

dnfclas commented May 17, 2016

@Dewaldf, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

…t the attribute is not checked for every grain.

Fixed code formatting.
isManagementFilterableGrain =
data.GrainInstanceType.IsDefined(typeof (ManagementFilterableGrain), false);
//we need to add the new type to the dictionary now,so that we do not need to get attribute for everygrain.
if (!checkedTypes.ContainsKey(data.GrainInstanceType))
Copy link
Contributor

Choose a reason for hiding this comment

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

This second if doesn't seem necessary.

@gabikliot
Copy link
Contributor

Not sure why you need this ManagementFilterableGrain.
Instead pass type name string to the GetDetailedGrainStatistics function, to get counts only for grain of that type.
And we should have another mgmt function to retrieve all types.

Dewaldf and others added 2 commits May 18, 2016 08:05
Removed extra if in GetDetailedGrainStatistics method
…th the ManagementFilterableGrain attibute in the system.

Refactored the GetDetailedGrainStatistics method to use GetGrainTypeList method to get the types to return.
@Dewaldf
Copy link
Contributor Author

Dewaldf commented May 18, 2016

Just committed changes as suggested.

@gabikliot, The reason I decided to use the ManagementFilterableGrain attribute, is to make it easy for developers to get the grains they are interested in, the other option was to allow them to pass an array of Types into the GetDetailedGrainStatistics method, but that might get bloated if the developer wants to return a big list of grain types.

The GetGrainTypeList method that I added to the GrainTypeManager, can return a list of all grain types or we can filter out only the ManagementFilterable grains.

@gabikliot
Copy link
Contributor

You have to think about the API in general terms, for all future usages and not just for your one particular case. The caller needs an ability to decide what grains he is interested in. And not the callee, which is in this case the grain itself. Grain developer can not know at the time he is writing the grain which client of the management interface will want what.

For example, look atIEnumerable.Where. You pass the filter function as a parameter by the caller, and not the other way around, which would be in analogy with what you suggested to have the items of IEnumerable to implement IFilterable.

In our case we want (at least) 2 function: get all grain types and get grains for a given type. That way the portal (the client of the management API) has the full flexibility and can for example iterate over all types and pick which ones he is interested in.
It is basically a standard API design principle.

@@ -318,4 +318,9 @@ public ImplicitStreamSubscriptionAttribute(string streamNamespace)
Namespace = streamNamespace;
}
}

[AttributeUsage(AttributeTargets.Class)]
public sealed class ManagementFilterableGrain : Attribute
Copy link
Contributor

Choose a reason for hiding this comment

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

As pointed out in the review this interface is un-necessary.

Added methods to get all active grain types on the ManagementGrain
Changed methods to get active grains by passing in the types the developer would be interested in
@Dewaldf
Copy link
Contributor Author

Dewaldf commented May 19, 2016

Committed the suggested changes.

/// <param name="hostsIds">List of silos this command is to be sent to.</param>
/// <param name="types">Array of grain types to filter the results with</param>
/// <returns></returns>
Task<DetailedGrainStatistic[]> GetDetailedGrainStatistics(SiloAddress[] hostsIds, string[] types);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to have a single method with default arguments?

GetDetailedGrainStatistics(string[] types = null, SiloAddress[] hostsIds=null)

where nulls mean all types and all silos respectively?

I would also flip the order of arguments assuming that specifying grain types would be a more common case than specifying a list of silos. That way GetDetailedGrainStatistics() and GetDetailedGrainStatistics(grainTypes) would be convenient shortcuts.

@sergeybykov sergeybykov self-assigned this May 24, 2016
@Dewaldf
Copy link
Contributor Author

Dewaldf commented May 24, 2016

I had the methods the way you suggested in the last comment, but then changed it because it was not consistent with the GetSimpleGrainStatistics method. I thought this was the way you guys preferred to have the overloads. Will make the changes shortly. :-)

@sergeybykov
Copy link
Contributor

Sorry if I confused you. I didn't realize that was how it was already done. I still think a single method is better than 4 overloads. :-)

…ults of null.

Changed the order of the parameter so that types is the firt param
Changed GetActiveGrainTypes to be one method with default params instead of overload.
@Dewaldf
Copy link
Contributor Author

Dewaldf commented May 25, 2016

No worries, I have just committed the changes as suggested.

GetActiveGrainTypes(SiloAddress[] hostsIds=null)
GetDetailedGrainStatistics(string[] types = null,SiloAddress[] hostsIds=null)

@sergeybykov sergeybykov merged commit aa7fc5d into dotnet:master May 25, 2016
@sergeybykov
Copy link
Contributor

Thank you, @Dewaldf!

Did you think about exposing this functionality via OrleansManager.exe?

@Dewaldf
Copy link
Contributor Author

Dewaldf commented May 26, 2016

Hi,
This might be a bit late. We do not have a requirement to add this functionality to OrleansManager.exe. If you think this will add value then I will gladly add it there.

@sergeybykov
Copy link
Contributor

I think it'd be better to keep OrleansManager.exe in sync with the ManagementGrain API. Thanks!

@sergeybykov sergeybykov mentioned this pull request Oct 3, 2016
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants