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

Remove initialized=false if a configuration fails to load #220

Merged
merged 2 commits into from Jul 7, 2017

Conversation

Projects
None yet
4 participants
@akonsand
Contributor

akonsand commented Jun 24, 2017

In some cases the key value fetch fails, due to consul being down for a few seconds, or the connection failing, in which case this flag is set to false. If it is set to false, the getConfiguration() method keeps on throwing IllegalStateExceptions, bringing our entire application down.

I think the purpose of the initialized flag was to to prevent getConfiguration() being called before init() completed, and hence setting the flag here is not needed.

@akonsand

This comment has been minimized.

Show comment
Hide comment
@akonsand
Contributor

akonsand commented Jun 24, 2017

@jtroxel

This comment has been minimized.

Show comment
Hide comment
@jtroxel

jtroxel Jul 5, 2017

👍 this fixes an issue for me as well

jtroxel commented Jul 5, 2017

👍 this fixes an issue for me as well

@norbertpotocki norbertpotocki merged commit c1caeea into cfg4j:master Jul 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@norbertpotocki

This comment has been minimized.

Show comment
Hide comment
@norbertpotocki

norbertpotocki Jul 7, 2017

Member

Thanks for contributing this @akonsand!

Member

norbertpotocki commented Jul 7, 2017

Thanks for contributing this @akonsand!

norbertpotocki added a commit that referenced this pull request Jul 12, 2017

Remove initialized=false if a configuration fails to load (#220)
* Remove initialized=false if a configuration fails to load

norbertpotocki added a commit that referenced this pull request Jul 12, 2017

@LostOblivion

This comment has been minimized.

Show comment
Hide comment
@LostOblivion

LostOblivion Jul 31, 2017

Hi, great framework. :)

But we have the same problem with GitConfigurationSource.

LostOblivion commented Jul 31, 2017

Hi, great framework. :)

But we have the same problem with GitConfigurationSource.

@norbertpotocki

This comment has been minimized.

Show comment
Hide comment
@norbertpotocki

norbertpotocki Jul 31, 2017

Member

Hi @LostOblivion. This issue has been fixed in release 4.4.1. Let me know if you're still running into it.

Member

norbertpotocki commented Jul 31, 2017

Hi @LostOblivion. This issue has been fixed in release 4.4.1. Let me know if you're still running into it.

@LostOblivion

This comment has been minimized.

Show comment
Hide comment
@LostOblivion

LostOblivion Aug 1, 2017

@norbertpotocki Is it fixed for GitConfigurationSource, or just for ConsulConfigurationSource? We still get the error with 4.4.1 when using GitConfigurationSource.

Perhaps this line is the problem?

https://github.com/cfg4j/cfg4j/blob/master/cfg4j-git/src/main/java/org/cfg4j/source/git/GitConfigurationSource.java#L164

LostOblivion commented Aug 1, 2017

@norbertpotocki Is it fixed for GitConfigurationSource, or just for ConsulConfigurationSource? We still get the error with 4.4.1 when using GitConfigurationSource.

Perhaps this line is the problem?

https://github.com/cfg4j/cfg4j/blob/master/cfg4j-git/src/main/java/org/cfg4j/source/git/GitConfigurationSource.java#L164

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