Comcast / jrugged

A Java libary of robustness design patterns
Apache License 2.0
266 stars 93 forks source link

Incomplete API in Initializer/Initializable #18

Closed ghost closed 10 years ago

ghost commented 11 years ago

If the Initializer stops due to exceeding the maximum number of retries, there appears to be no API to detect that situation.

joercampbell commented 11 years ago

Are you thinking there should be a notification interface? You can always ask the initializer if it is initialized (the boolean flag would indicate that) and you can also fetch from it the number of attempts that have been made. It doesn't currently have a way to get what the 'max' attempts is set to. Can you describe for me a little more what type of API you were looking for here?

ghost commented 11 years ago

Having to repeated poll the initializer to see if it is initialized and having to check the number of retry attempts seems contrary to the whole purpose of the jrugged library as I understand it. Some sort of notification/callback mechanism sounds good to me.

A quick and dirty hack I did to get something working for a project was to add a maxRetriesAttempted method to the Initializable interface and add the following code to the bottom of the run method in the Initializer class:

    if(!initialized && (numAttempts >= maxRetries) && !cancelled) {
        client.maxRetriesAttempted();
    }

It worked for my purposes at the time, but I don't claim it is an ideal solution.

joercampbell commented 11 years ago

What about this:

the afterInit() call gets an ENUM that represents the current 'state' - like RETRIES_EXHAUSTED, INITIALIZED... etc. so that the client knows the state and can act on it.

So I would move the after init call OUTSIDE the while loop - so that you either get an exception related to startup or your get to a point where I am going to 'tell' you the result of the attempts to do so.

ghost commented 11 years ago

That would work. However, to me a method name of afterInit implies that the initialization has completed successfully. Hence using it to handle the case of an initialization failure/timeout seems contrary to the method name. My preference would be to leave afterInit for successful initializations and have a separate method for initialization failures.

joercampbell commented 11 years ago

Hmmm - then maybe its the 'name' of the method that matters. I read afterInit as - after the actions I expect have been completed I will tell you what State I am in... So maybe a rename of that method to something like afterInitAttempt(Enum State) to get the results you are looking for.

Perhaps to - I am thinking about the use case you have differently than you are. I believe you are saying you want to have two different methods as 'callbacks' - rather than a single call back TELLING you what state the init is in?

ghost commented 11 years ago

I do a fair amount of Ajax programming and typically one has both a success callback and a failure callback for the Ajax calls. I like that separation and thus my preference for that approach, but I could live with using afterInitAttempt(Enum state).

Are you concerned with backwards compatibility? It isn't a problem for me if you were to change the method name and signature, but perhaps it is for others?

joercampbell commented 10 years ago

Thanks - I took your first suggestion and put it into master - Should go with our next release. Which I want to get out very soon. Thanks for the discussion and submission.