Question about usage

[Wesam18]

We are thinking to use Mjolnir in our applicaiton and we would like to know about how many companies are using it now?. Is it porduciton ready code?. Are you willing to continue contributing to the project?

[robhruska]

We currently use it in production - I haven't gotten feedback from other companies that are also using it, but would love to hear them reply about it here if they are!

Definitely interested in continuing to contribute. In fact, I've been doing quite a bit of work on the next version (2.6.0) that addresses some of the things I didn't like about how I originally used async and thread pools for certain things.

#46 and #47 are the upcoming PRs. I don't have a timeline on them, but I anticipate they'll be released within the next couple months. I've sidelined them for just a bit while working on a couple other things, but plan to pick them up at the beginning of the new year.

Happy to answer other questions you might have, and always receptive to feedback.

[senj]

I think the documentation and new interface doesn't really match. I'm looking forward to your PR, I'm planning to use it, but not if I'm forced to implement sync methods.

[robhruska]

I think the documentation and new interface doesn't really match.

True - still working through some things and then I'll get the docs updated.

but not if I'm forced to implement sync methods.

There's no intent to force sync implementations (you'd just be able to extend AsyncCommand without a corresponding SyncCommand implementation). However, if you had code where you needed the same functionality but on both sync and async code paths, you'd probably want to implement both.

I'm interested in hearing more on your thoughts there. My take is that (and this seems to be the community/expert consensus) converting between sync/async (especially async-up-to-sync) is generally ill-advised (since you lose a lot of the async advantages), which is the driving force for needing two separate implementations (i.e. the underlying behavior of the sync one will differ from the async).

[senj]

Yes don't get me wrong, the PR looks good, I meant the current implementation with ICommand and Invoke / InvokeAsync

Edit: Sorry my fault, I need to use Command, not ICommand.

Imagine we want to write a REST Service and secure it with mjolnir (great!).
If something goes wrong and I catch the command exception in my repository or use the fallback method which I would prefer, there is no easy way to check in my controller if an error occured or if the call was successful. (please correct me if I'm wrong).
What do you think of this approach? http://danielwestheide.com/blog/2012/12/26/the-neophytes-guide-to-scala-part-6-error-handling-with-try.html

[robhruska]

there is no easy way to check in my controller if an error occured or if the call was successful. (please correct me if I'm wrong).

That's true with the current API (inheriting from Command).

I like the Try approach, and #46 is moving in that direction (although admittedly without near as much functionality as as Try provides). I'll experiment with a couple different options to that end, though.

Question: how important is it for you that fallback behavior is provided by Mjolnir vs. just coding the fallbacks in your own code? I've been scoping them out of this rework, but am open to re-including them. My hope was that the introduction of CommandResult and the InvokeReturn*() overloads would make writing fallbacks easier (since it'd help avoid significant try/catch boilerplate).

Thanks for your feedback, this is super useful.

[senj]

Thought about it and made a little, easy POC...

-- Command --
I missed the possibility to get more information back than value and exception. Such a dictionary is not the best solution, but I need something to store information in.

public class ServiceCallCommand<T> : Command<CommandResult<T>>
    {
        private readonly string _url;

        public ServiceCallCommand(string url) : base("group", "isolationKey", TimeSpan.FromSeconds(30))
        {
            _url = url;
        }

        protected override async Task<CommandResult<T>> ExecuteAsync(CancellationToken cancellationToken)
        {
            HttpResponseMessage response = null;
            CommandResult<T> cresult = new CommandResult<T>();

            using (HttpClient client = new HttpClient())
            {
                try
                {
                    response = await client.GetAsync(_url);
                    cresult.value = await response.Content.ReadAsAsync<T>();
                }
                catch (Exception ex)
                {
                    cresult.exception = ex;
                    cresult.store.Add("statusCode", response?.StatusCode);
                }
            }

            return cresult;
        }
    }

    public class CommandResult<TResult>
    {
        public Dictionary<string, object> store = new Dictionary<string, object>();

        public TResult value { get; set; }

        public Exception exception { get; set; }

        public bool IsSuccessful => exception == null;
    }

-- Repository --
Very easy, there is no exception handling or something, only get that command back.
If we have to work with it, we can use command.IsSuccessful and command.value to work on the value.

public async Task<CommandResult<string>> Get(string url)
        {
            ServiceCallCommand<string> command = new ServiceCallCommand<string>(url);
            return await command.InvokeAsync();
        }

-- Controller --
Then it's very easy to determine which return code I have. At the moment, we have
value != null -> 200 Ok
value == null -> 404 NotFound
try/catch of _repository.Get() throws exception -> 500 Internal Server Error

This is not a good solution. I want the exact error code the origin request caused.

CommandResult<string> result = await _repository.Get(url);

            if(result.IsSuccessful)
            {
                _logger.LogInformation("Returning " + result);
                return Ok(result);
            }
            else
            {
                _logger.LogInformation("Returning " + result);

                object statusCode = 0;
                if (result.store.TryGetValue("statusCode", out statusCode))
                {
                    return new HttpStatusCodeResult((int) statusCode);
                }
                else
                {
                    throw result.exception;
                }
            }

The drawback is of cause that I have CommandResult everywhere.
(and have to cast my additional information.)

[robhruska]

I want the exact error code the origin request caused.

I didn't quite follow this statement - it looks like an exception thrown from the repository would be one that probably wouldn't have an HTTP response status and rather be something like a connection error (that wouldn't have a status code anyway).

You could also consider a more specific HttpResult type and return a CommandResult<HttpResult<T>> from your command, and/or perhaps "unwrap" the CommandResult in the repository and have it return an HttpResult (so that callers of the repository have a cleaner API and don't have to have knowledge of the structure of store for varying types of requests).

The drawback is of cause that I have CommandResult everywhere.

Yeah, that was a sticking point for me as well with the new API, and my hope is that callers will "unwrap" at their point of execution to avoid it propagating out throughout their code. Not sure how well that applies in practice, though.

[senj]

That's a good point, thank you. I knew I took too much of "my" logic in the command.

[Wesam18]

Hi @robhruska

Thank you for answering my question. We are waiting for the new release. We have postponed the integration with Mjolnir until you release the new version so we can use the new mechanisms.

Are you going to release it soon?

[robhruska]

Let me get back to you on that after this weekend, I'll see if I can tie a bow on that release. It's pretty close, just needs a couple more tasks finished and some testing.

[robhruska]

@Wesam18 I've pushed a prerelease version of 2.6.0 up to nuget.org. You can install it via the NuGet package manager dialog, or via the package manager console using install-package Hudl.Mjolnir -version 2.6.0-next149.

It should be pretty stable. Unit tests pass, and some high-level soak tests also look good. However, I still have tests to run on our staging environments, and need to get it deployed to some of our services. Once I've proven it on some of our production systems I'll push the official 2.6.0 to NuGet. If you run into any problems, please file issues here on GitHub so I can get them into the official release.

The Wiki has been updated with full 2.6.0 documentation. https://github.com/hudl/Mjolnir/wiki

I'd appreciate any feedback or suggestions you have along the way!

[Wesam18]

Thank you very much @robhruska

[sds88]

@robhruska Any updates on releasing 2.6.0?

[robhruska]

I had to sideline it for a bit. We've still got some testing to do in-house in our production environment before I'm comfortable saying that everything's good to go for 2.6.0.

Out of curiosity, have you tried the 2.6.0 RC? If so, I'd be interested in knowing if it's worked in any testing you've done to this point.

I'll make it higher priority to get it shipped. Apologies for the delay. If all goes well with our testing, I can try and have it out within a few weeks. I haven't made any changes to the RC, so if testing and production deployment go well, that's the package that'll be released if you want to use it in the meantime.

[sds88]

We've been using the RC build for a few weeks with only one command type set up. So far it seems to be great, except for bulkhead config events not firing (I made a separate issue about that #53). The new metrics that you created are a lot more useful then the old stats.

[robhruska]

As 2.6.0 has been released, I'm going to close this one out. Feel free to reopen (or open another issue) for additional discussion.