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

Add Memory Stream Provider #2063

Merged
merged 16 commits into from
Aug 22, 2016
Merged

Add Memory Stream Provider #2063

merged 16 commits into from
Aug 22, 2016

Conversation

ancienticybeing
Copy link
Contributor

@ancienticybeing ancienticybeing commented Aug 15, 2016

Add Memory Stream Provider, Would you mind squashing the commits for me? Thanks.

@dnfclas
Copy link

dnfclas commented Aug 15, 2016

Hi @JingqiaoFu, 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;

@gabikliot
Copy link
Contributor

Is it mimicking Azure Queue provider or Event Hub? Grain per queue?

{
public interface IMemoryStreamQueueGrain : IGrainWithGuidKey
{
Task SetMaxEventCount(int maxEventCount);
Copy link
Contributor

@gabikliot gabikliot Aug 17, 2016

Choose a reason for hiding this comment

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

who calls SetMaxEventCount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is supposed to be a helper function for users that limits the size of the queue. It doesn't have to be called during the provider's life cycle.

@gabikliot
Copy link
Contributor

I am not sure you if you meant that you addressed my feedback. It does not appear so. In particular, I mean my main feedback about a different design that will build an in memory emulation of AQ or EH, and not the full new adapter.

@ancienticybeing ancienticybeing changed the title Added Memory Stream Provider Add Memory Stream Provider Aug 18, 2016
@ancienticybeing
Copy link
Contributor Author

ancienticybeing commented Aug 18, 2016

Gaby: Sorry about the confusion, the fixes I made was for the internal codeflow feedback.

@gabikliot
Copy link
Contributor

I did not realize @JingqiaoFu is on the core team, I thought he needs my feedback.
No problem, scratch out my feedback.

int providerNameGuidHash = (int)jenkinsHash.ComputeHash(providerName);

// get queueId hash code
uint queueIdHash = queueId.GetUniformHashCode();
Copy link
Member

Choose a reason for hiding this comment

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

in order to trust this uniform hash code, please remove the overloads for QueueId.GetQueueId that do not take an explicit hash. The currently have no usages, so it's an easy removal, but will prevent naive usages in the future.

}
catch (Exception exc)
{
throw;
Copy link
Member

Choose a reason for hiding this comment

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

if this catch is just throwing, why are you catching in the first place? Didn't you forget a log call here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there should be an error logging call. Will add that.

… QueueId.GetUniformHashCode(). Added Error logging in MemoryAdapterReceiver.GetQueueMessagesAsync
@jason-bragg jason-bragg merged commit e03cab4 into dotnet:master Aug 22, 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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants