Mailing-List: contact reviews-help@aurora.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.apache.org
Content-Type: multipart/alternative;
 boundary="===============1712157602380949190=="
MIME-Version: 1.0
Subject: Re: Review Request 45511: AURORA-1493: create ELB-friendly endpoint
 to
 detect leading scheduler
From: Ashwin Murthy <ashwinmurthy@gmail.com>
To: Bill Farner <wfarner@apache.org>
Cc: Joshua Cohen <jcohen@apache.org>, Aurora <reviews@aurora.apache.org>,
 Stephan Erb <serb@apache.org>, Ashwin Murthy <ashwinmurthy@gmail.com>,
 Aurora ReviewBot <wfarner@apache.org>
Date: Thu, 31 Mar 2016 21:54:52 -0000
Message-ID: <20160331215452.5121.61514@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: Ashwin Murthy <noreply@reviews.apache.org>
References: <20160331183341.23781.76453@reviews.apache.org>
In-Reply-To: <20160331183341.23781.76453@reviews.apache.org>
Reply-To: Ashwin Murthy <ashwinmurthy@gmail.com>

--===============1712157602380949190==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> On March 31, 2016, 6:33 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/LeaderHealth.java, line 49
> > <https://reviews.apache.org/r/45511/diff/4/?file=1320481#file1320481line49>
> >
> >     Can we switch on this enum, rather than if/else branching? If we also add an explicit case for each enum value and reserve the default block for an unexpected status it will allow this code to continue to work as expected even if new values are added to the enum in the future (i.e. as things stand today, if we added a hypothetical state like `LeaderStatus.ALSO_LEADING`, it would fall into the `SERVICE_UNAVAILABLE` branch).
> 
> Ashwin Murthy wrote:
>     @Bill, @Joshua, 
>     
>     Can I do the following?
>     
>     1. Return 200 is leader
>     2. Return 501 (Not implemented) if this is not the leader but a leader exists
>     3. Return 503 (Service unavailable) if there is no current leader
>     4. Return 500 (Internal server error) for the default case that Joshua points out
>     
>     Would you be ok with this? I
> 
> Joshua Cohen wrote:
>     In the [LeaderRedirectFilter](https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/http/LeaderRedirectFilter.java#L70-L91) we do the following:
>     
>     Is Leading -> continue request (i.e. 200)
>     No Leader -> 503
>     Not Leading -> 307
>     Unknown State -> 500
>     
>     So that meshes pretty well with what you describe, you're just replacing the redirect when the instance is not the leader with a non 2xx. I'm not sure if I love 501 for that case, but I'm having a hard time finding a better alternative.
>     
>     That being said, I feel like I've asked this before, but you can clarify why the existing `LeaderRedirectFilter` is not sufficient for ELB? If I'm reading the [docs](http://docs.aws.amazon.com/ElasticLoadBalancing/latest/DeveloperGuide/elb-healthchecks.html#health-check-configuration) right, it seems like any non-200 reply is sufficient to indicate the instance is not healthy (i.e. not the leader). So given that `LeaderRedirectFilter` already returns a 307 for this case, can we not just use that?

@Bill, @Joshua, 

Actually, I changed my mind. The prev one is rather more complex for this simple scenario than needs to be.

How about?:
Return 200 if instance is leader
Return 503 (Service unavailablE) for the case when this instance is not the leader but a leader exists
Return 500 (internal server error) for all other (default) cases since this is truly an error. 

Would you be ok with this?


- Ashwin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45511/#review126394
-----------------------------------------------------------


On March 31, 2016, 5:06 a.m., Ashwin Murthy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45511/
> -----------------------------------------------------------
> 
> (Updated March 31, 2016, 5:06 a.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> AURORA-1493: create ELB-friendly endpoint to detect leading scheduler. The fix is to add a new endpoint - "/leaderhealth" which returns http status code 200 (OK) if the instance is the leader. If the instance is not the leader but a leading exists, returns 500 (Internal server error). If there is no leader at all, returns 503 (Service unavailable)
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/http/LeaderHealth.java PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/LeaderHealthTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45511/diff/
> 
> 
> Testing
> -------
> 
> Added new unit test
> 
> 
> Thanks,
> 
> Ashwin Murthy
> 
>


--===============1712157602380949190==--