Keep an endpoint's healthiness if long-poll health check returns NOT_…

[imasahiro]

…MODIFIED status code

[codecov]

Codecov Report

Merging #2004 into master will increase coverage by 0.05%.
The diff coverage is 50%.

@@             Coverage Diff              @@
##             master    #2004      +/-   ##
============================================
+ Coverage     73.41%   73.47%   +0.05%     
- Complexity     9213     9222       +9     
============================================
  Files           810      810              
  Lines         35853    35856       +3     
  Branches       4404     4405       +1     
============================================
+ Hits          26322    26345      +23     
+ Misses         7258     7233      -25     
- Partials       2273     2278       +5

Impacted Files	Coverage Δ	Complexity Δ
...client/endpoint/healthcheck/HttpHealthChecker.java	85.29% <50%> (+0.67%)	18 <0> (+1)	⬆️
...lient/circuitbreaker/CircuitBreakerHttpClient.java	69.69% <0%> (-3.04%)	10% <0%> (-1%)
.../linecorp/armeria/internal/Http2GoAwayHandler.java	53.84% <0%> (-2.57%)	13% <0%> (-1%)
...rp/armeria/common/stream/DefaultStreamMessage.java	90% <0%> (-1.43%)	54% <0%> (-1%)
.../main/java/com/linecorp/armeria/server/Server.java	81.58% <0%> (-1.26%)	26% <0%> (ø)
...inecorp/armeria/server/grpc/ArmeriaServerCall.java	87% <0%> (-1.19%)	82% <0%> (ø)
...a/common/grpc/protocol/ArmeriaMessageDeframer.java	71.77% <0%> (+0.95%)	48% <0%> (+1%)	⬆️
.../linecorp/armeria/server/tomcat/TomcatService.java	65.95% <0%> (+2.65%)	23% <0%> (+1%)	⬆️
.../armeria/client/endpoint/dns/DnsEndpointGroup.java	86.31% <0%> (+3.15%)	15% <0%> (+2%)	⬆️
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 169f509...f50338d. Read the comment docs.

[minwoox]

Thanks!

[ikhoon]

I left a bit, but it doesn't matter. :-)
Thanks @imasahiro

[mauhiz]

Because he doesn't want to apply for whole 3XX range, I think

…

On Wed, Aug 21, 2019 at 10:31 AM Ikhun Um ***@***.***> wrote: ***@***.**** approved this pull request. I left a bit, but it doesn't matter. :-) Thanks @imasahiro <https://github.com/imasahiro> ------------------------------ In core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HttpHealthChecker.java <#2004 (comment)>: > @@ -108,8 +109,13 @@ private synchronized void check() { maxLongPollingSeconds = getMaxLongPollingSeconds(res); break; default: - // Do not use long polling on an unexpected status for safety. - maxLongPollingSeconds = 0; + if (res.status() == HttpStatus.NOT_MODIFIED) { nit: Why don’t you move this into a case statement to keep consistency. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2004?email_source=notifications&email_token=AAN34S2CDFH7OK7MFNBBTR3QFSLF5A5CNFSM4INLECG2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCCFJSCA#pullrequestreview-277518600>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN34S3HTMTIAFNNVP6MZM3QFSLF5ANCNFSM4INLECGQ> .

[minwoox]

Perhaps @ikhoon wants it like:

switch (res.status().codeClass()) {
    case SUCCESS:
        maxLongPollingSeconds = getMaxLongPollingSeconds(res);
        isHealthy = true;
        break;
	case REDIRECTION:
		if (res.status() == HttpStatus.NOT_MODIFIED) {
            maxLongPollingSeconds = getMaxLongPollingSeconds(res);
            isHealthy = wasHealthy;
        } else {
            // Do not use long polling on an unexpected status for safety.
            maxLongPollingSeconds = 0;
        }
    case SERVER_ERROR:
        maxLongPollingSeconds = getMaxLongPollingSeconds(res);
        break;
    default:
        // Do not use long polling on an unexpected status for safety.
        maxLongPollingSeconds = 0;
}

But I like the current way. :-)

[imasahiro]

Yes, if we need to handle more case, I would like to add the code (#2004 (comment)) but since 304 is the only special case we need to care at the moment, I would like to merge into default case.

[trustin]

Thanks, @imasahiro !