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

Proposal for failover design #631

Closed
asarkar opened this Issue Feb 8, 2017 · 13 comments

Comments

Projects
None yet
5 participants
@asarkar
Contributor

asarkar commented Feb 8, 2017

I'm looking at JGitEnvironmentRepository and pretty much everything happens in the refresh method. There are more than one problems with it:

  1. It clones the repo every time. See #566
  2. It doesn't have failover behavior.
  3. It is a private method, thus ruling out AOP that clients may want to do to handle the above items.

I propose that the method be made public, and that it falls back to using the local repo if remote clone fails. Question I've is, what version should be returned in that case? The one retrieved from local repo or something to indicate that the clone operation failed (I'm thinking UNKNOWN)?

Optionally, we could add a retry logic using project Reactor keeping it consistent with Spring 5, with configurable properties. This wouldn't be in my first pass, because making it public means the user can do AOP to attempt retries.

@ryanjbaxter

This comment has been minimized.

Contributor

ryanjbaxter commented Feb 8, 2017

This sounds useful. I suppose if you are going to fallback to using the local repo, you try and use the same version as what was requested from the remote repo. If that is not possible maybe it should just fail?

+1 to the retry logic as well, although I suggest you use Spring Retry, as that is what we are using in spring cloud netflix.

@asarkar

This comment has been minimized.

Contributor

asarkar commented Feb 8, 2017

@ryanjbaxter

If that is not possible maybe it should just fail?

I don't want it to fail no matter what. According to the 12-factor app, we must design keeping network problems in mind and failing config would in turn fail all apps that depend on it. That's not acceptable. What do you suggest returning the version in that case?

@spencergibb

This comment has been minimized.

Member

spencergibb commented Feb 8, 2017

In another issue, it was mentioned to enhance the CompositeEnvironmentRepository as a way to failover. Have a normal git repo, then a native one that could fall through as the backup.

There is retry already on the client side.

@asarkar

This comment has been minimized.

Contributor

asarkar commented Feb 8, 2017

@spencergibb I believe the retry on the client side is different. The client retries to connect to config server if not successful. AFAIK, there's no retry for a request that actually triggers the git clone and subsequent failure, which is what I'm talking of handling here.
What's the other issue you referred to?

@spencergibb

This comment has been minimized.

Member

spencergibb commented Feb 9, 2017

Yup, retry is client side. This is the issue #617 (comment)

@asarkar

This comment has been minimized.

Contributor

asarkar commented Feb 9, 2017

@spencergibb I read what's asked in #617. What I'm talking about here is serving from local repo if clone fails. 617 seems to be about having multiple Git repos. Note that in case of a network outage, all repos could be unavailable, so having one or more backups might not help. It also assumes (but doesn't explicitly mention) that the repos are configured in HA mode and replicating data.
Of course, you could do both as they are not mutually exclusive.

@spencergibb

This comment has been minimized.

Member

spencergibb commented Feb 9, 2017

I did say here and in the issue about having a fallback native repo. No network.

@asarkar

This comment has been minimized.

Contributor

asarkar commented Feb 9, 2017

@spencergibb I submitted #634 making JGitEnvironmentRepository#refresh public so that clients can do AOP while you work towards providing 1st-class support for failover. It is item 3 in my post.

@anrub

This comment has been minimized.

anrub commented Mar 20, 2017

@asarkar What do you mean with 2 - as far as I can see, the last checkout is returned, if the fetch is not successful, isn't it?

@asarkar

This comment has been minimized.

Contributor

asarkar commented May 5, 2017

@anrub That's not been my experience: If connection to Git fails, the service fails to start. Have you verified the behavior you stated above? Can you point me to the code that does so?

@ezraroi

This comment has been minimized.

ezraroi commented Mar 20, 2018

@anrub looks like the refresh method of the JGitEnvironmentRepository will throw exception and the call fetch configuration will fail. Any progress or solution for GitHub not available?

@spencergibb

This comment has been minimized.

Member

spencergibb commented Sep 17, 2018

#1 (see #1001) and #3 have been addressed. Is there anything else that needs to be done for Num 2?

@spencergibb

This comment has been minimized.

Member

spencergibb commented Oct 1, 2018

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment