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 retry for dynamic content #1166

Closed
thomasmodeneis opened this Issue May 25, 2015 · 15 comments

Comments

Projects
None yet
4 participants
@thomasmodeneis

Hi,

getJson often get into an error that I believe is because HTTP server sometimes may return 404 or a wrong content, this is being parsed and the error is below:

ERROR: 2015/05/25 Cannot read json from resource http://127.0.0.1:3000/123test with error message invalid character 'E' looking for beginning of value
ERROR: 2015/05/25 Error while rendering page 123test.md: reflect: call of reflect.Value.Type on zero Value

Is there a retry policy for the dynamic content ?

Thanks

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep May 25, 2015

Member

Don't thinks there's any retry policy.

/cc @SchumacherFM

Member

bep commented May 25, 2015

Don't thinks there's any retry policy.

/cc @SchumacherFM

@SchumacherFM

This comment has been minimized.

Show comment
Hide comment
@SchumacherFM

SchumacherFM May 26, 2015

Contributor

There is no retry policy.

Contributor

SchumacherFM commented May 26, 2015

There is no retry policy.

@SchumacherFM

This comment has been minimized.

Show comment
Hide comment
@SchumacherFM

SchumacherFM May 26, 2015

Contributor

Just found this: https://github.com/sethgrid/pester
I could implement it with a default retry of 2secs and a log warning that the retry is happening.
What do you think @bep ?

Contributor

SchumacherFM commented May 26, 2015

Just found this: https://github.com/sethgrid/pester
I could implement it with a default retry of 2secs and a log warning that the retry is happening.
What do you think @bep ?

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep May 26, 2015

Member

Hmm ... Hugo gets some deserved critique about having too many external dependencies ... I do not think this one reaches the bar of inclusion ( @spf13 may disagree). A simple, hardcoded retry n times with a pause would be welcome, though.

Member

bep commented May 26, 2015

Hmm ... Hugo gets some deserved critique about having too many external dependencies ... I do not think this one reaches the bar of inclusion ( @spf13 may disagree). A simple, hardcoded retry n times with a pause would be welcome, though.

@bep bep added the Enhancement label May 26, 2015

@bep bep changed the title from Dynamic Content - Cannot read json from resource to Add retry for dynamic content May 26, 2015

@spf13

This comment has been minimized.

Show comment
Hide comment
@spf13

spf13 May 26, 2015

Contributor

Agree with bep. Keep it simple. 

Best,
Steve

Steve Francia
http://stevefrancia.com
http://spf13.com
http://twitter.com/spf13

On Tue, May 26, 2015 at 12:48 AM -0700, "Bjørn Erik Pedersen" notifications@github.com wrote:

Hmm ... Hugo gets some deserved critique about having too many external dependencies ... I do not think this one reaches the bar of inclusion ( @spf13 may disagree). A simple, hardcoded retry n times with a pause would be welcome, though.


Reply to this email directly or view it on GitHub.

Contributor

spf13 commented May 26, 2015

Agree with bep. Keep it simple. 

Best,
Steve

Steve Francia
http://stevefrancia.com
http://spf13.com
http://twitter.com/spf13

On Tue, May 26, 2015 at 12:48 AM -0700, "Bjørn Erik Pedersen" notifications@github.com wrote:

Hmm ... Hugo gets some deserved critique about having too many external dependencies ... I do not think this one reaches the bar of inclusion ( @spf13 may disagree). A simple, hardcoded retry n times with a pause would be welcome, though.


Reply to this email directly or view it on GitHub.

@thomasmodeneis

This comment has been minimized.

Show comment
Hide comment
@thomasmodeneis

thomasmodeneis May 26, 2015

I also like the idea of simple retry with no external deps, just to make it simpler. I think adding warn a message with the status code (in case of failures) will also help debug possible problems :)

I also like the idea of simple retry with no external deps, just to make it simpler. I think adding warn a message with the status code (in case of failures) will also help debug possible problems :)

@SchumacherFM

This comment has been minimized.

Show comment
Hide comment
@SchumacherFM

SchumacherFM May 27, 2015

Contributor

I'll build something and send a PR for review.

BTW: Dave Cheney mentioned yesterday hugo at his presentation at Sydney Go Meetup: Why hugo must use etcd libraries? So many dependencies ...

(Of course due to viper ...)

Contributor

SchumacherFM commented May 27, 2015

I'll build something and send a PR for review.

BTW: Dave Cheney mentioned yesterday hugo at his presentation at Sydney Go Meetup: Why hugo must use etcd libraries? So many dependencies ...

(Of course due to viper ...)

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep May 27, 2015

Member

Mr. Cheney just wants to sell his GB framework :-) About the etcd framework he's got a big point. There should be a way for us to opt-out of that feature.

PR welcome.

Member

bep commented May 27, 2015

Mr. Cheney just wants to sell his GB framework :-) About the etcd framework he's got a big point. There should be a way for us to opt-out of that feature.

PR welcome.

@spf13

This comment has been minimized.

Show comment
Hide comment
@spf13

spf13 May 27, 2015

Contributor

I agree about Viper needing to break up a bit. If we move the remote dependencies into another library it would keep things simple. Challenge is doing that and keeping the batteries included feel it has today in Go.

Contributor

spf13 commented May 27, 2015

I agree about Viper needing to break up a bit. If we move the remote dependencies into another library it would keep things simple. Challenge is doing that and keeping the batteries included feel it has today in Go.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep May 27, 2015

Member

Well, what Go should have had was a sense of enabling features from the project/client side by turning on features by adding dependencies to the GOPATH.

Is etcd in GOPATH? Enable that feature.

In the Java world, the entire Spring Boot project is built upon this concept.

Member

bep commented May 27, 2015

Well, what Go should have had was a sense of enabling features from the project/client side by turning on features by adding dependencies to the GOPATH.

Is etcd in GOPATH? Enable that feature.

In the Java world, the entire Spring Boot project is built upon this concept.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep May 27, 2015

Member

Failing that, Viper should maybe come in two variants -- an all batteries included and a bare-bone version.

Member

bep commented May 27, 2015

Failing that, Viper should maybe come in two variants -- an all batteries included and a bare-bone version.

@SchumacherFM

This comment has been minimized.

Show comment
Hide comment
@SchumacherFM

SchumacherFM May 27, 2015

Contributor

Yes a bare bone Viper version where I can add features from other Viper packages. These features simply fullfill an interface in Viper.
I'm currently also using Viper in an eCommerce project ... but I hide Viper in a private struct field because I may replace Viper with a slimmer solution.

And gb is awesome :-)

Contributor

SchumacherFM commented May 27, 2015

Yes a bare bone Viper version where I can add features from other Viper packages. These features simply fullfill an interface in Viper.
I'm currently also using Viper in an eCommerce project ... but I hide Viper in a private struct field because I may replace Viper with a slimmer solution.

And gb is awesome :-)

SchumacherFM added a commit to SchumacherFM/hugo that referenced this issue May 28, 2015

Fix #1166 Add 1x retry with 2sec sleep time. (+Tests)
The retry gets triggered when the parsing of the content fails.
JSON unmarshal fails due to invalid JSON -> retry
CSV cannot find separator -> retry
CSV failed to parse -> retry

HTTP Response from host gets not checked nor there is a timeout
configured for the default http client.
@SchumacherFM

This comment has been minimized.

Show comment
Hide comment
@SchumacherFM

SchumacherFM May 28, 2015

Contributor

@thomasmodeneis please see PR #1175 You can also checkout my branch, compile hugo and then test it.

Contributor

SchumacherFM commented May 28, 2015

@thomasmodeneis please see PR #1175 You can also checkout my branch, compile hugo and then test it.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep May 28, 2015

Member

Re. Viper, see spf13/viper#79

I hadn't thought about "blank imports".

Member

bep commented May 28, 2015

Re. Viper, see spf13/viper#79

I hadn't thought about "blank imports".

SchumacherFM added a commit to SchumacherFM/hugo that referenced this issue May 29, 2015

Fix #1166 Add 1x retry with 2sec sleep time. (+Tests)
The retry gets triggered when the parsing of the content fails.
JSON unmarshal fails due to invalid JSON -> retry
CSV cannot find separator -> retry
CSV failed to parse -> retry

HTTP Response from host gets not checked nor there is a timeout
configured for the default http client.

SchumacherFM added a commit to SchumacherFM/hugo that referenced this issue May 30, 2015

Fix #1166 Add 1x retry with 2sec sleep time. (+Tests)
The retry gets triggered when the parsing of the content fails.
JSON unmarshal fails due to invalid JSON -> retry
CSV cannot find separator -> retry
CSV failed to parse -> retry

HTTP Response from host gets not checked nor there is a timeout
configured for the default http client.

Fix tests because missing directory separator

Fix missing file Close(). Fix bug in GetCSV

Fix incorrect f.Close(); Use defer Luke!

SchumacherFM added a commit to SchumacherFM/hugo that referenced this issue May 31, 2015

GetJSON/GetCSV: Add 1x retry with 2sec sleep time
Fixes #1166

The retry gets triggered when the parsing of the content fails.
JSON unmarshal fails due to invalid JSON -> retry
CSV cannot find separator -> retry
CSV failed to parse -> retry

HTTP Response from host gets not checked nor there is a timeout
configured for the default http client.

Fix tests because missing directory separator

Fix missing file Close(). Fix bug in GetCSV

Fix incorrect f.Close(); Use defer Luke!

@bep bep closed this in cc5d63c Jun 1, 2015

@thomasmodeneis

This comment has been minimized.

Show comment
Hide comment
@thomasmodeneis

thomasmodeneis Jun 7, 2015

@SchumacherFM @bep @spf13 I've tested this several times and it works great...perfect! :)
Thanks

@SchumacherFM @bep @spf13 I've tested this several times and it works great...perfect! :)
Thanks

tychoish added a commit to tychoish/hugo that referenced this issue Aug 13, 2017

GetJSON/GetCSV: Add retry on invalid content
The retry gets triggered when the parsing of the content fails.

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