-
Notifications
You must be signed in to change notification settings - Fork 18
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
Added Rotation Management #63
Changes from 9 commits
29eb85d
a0ffdd8
e88fac0
96e0f9a
282db60
c8ce346
e549aa3
bb6cfd4
5816338
6d74d24
2bb44b6
b7fb378
ec9e44e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
package com.flipkart.gjex.core.config; | ||
|
||
import com.flipkart.gjex.core.web.RotationManagementResource; | ||
import org.glassfish.jersey.server.ResourceConfig; | ||
|
||
import javax.inject.Inject; | ||
import javax.inject.Named; | ||
import javax.inject.Singleton; | ||
|
||
/** | ||
* RotationManagementConfig for registering RotationManagementResource. | ||
* @author ajay.jalgaonkar | ||
* | ||
*/ | ||
|
||
@Singleton | ||
@Named("RotationManagementConfig") | ||
public class RotationManagementConfig extends ResourceConfig { | ||
|
||
@Inject | ||
public RotationManagementConfig (RotationManagementResource rotationManagementResource) { | ||
register(rotationManagementResource); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
package com.flipkart.gjex.core.task; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RotationManagementBasedHealthCheck does not belong to core/task package . Can you please create a healthcheck package under code and move healthcheckRegistry.java as well to it . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved |
||
|
||
import com.flipkart.gjex.core.logging.Logging; | ||
import io.dropwizard.metrics5.health.HealthCheck; | ||
|
||
import javax.inject.Singleton; | ||
import java.util.concurrent.atomic.AtomicBoolean; | ||
|
||
/** | ||
* Rotation management based health check for the app | ||
* @author ajay.jalgaonkar | ||
*/ | ||
|
||
@Singleton | ||
public class RotationManagementBasedHealthCheck extends HealthCheck implements Logging { | ||
private static final String BIR = "bir"; | ||
private static final String OOR = "oor"; | ||
|
||
private AtomicBoolean state; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we refactor state attribute to maybe inRotation to signify what it will actually store . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. refactored to rotationStatus There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please make this AtomicBoolean a final as well . We do not require more than 1 instance of this attribute There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
||
public RotationManagementBasedHealthCheck() { | ||
state = new AtomicBoolean(true); | ||
} | ||
|
||
@Override | ||
protected Result check() { | ||
if (isBir()) { | ||
info("Returning healthy status."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel these info logs are not necessary for every health check request. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed |
||
return Result.healthy("Server is " + getStatus()); | ||
} else { | ||
info("Returning unhealthy status."); | ||
return Result.unhealthy("Server is " + getStatus()); | ||
} | ||
} | ||
|
||
public String getStatus() { | ||
return isBir() ? BIR : OOR; | ||
} | ||
|
||
public String makeOor() { | ||
state.set(false); | ||
return OOR; | ||
} | ||
|
||
public String makeBir() { | ||
state.set(true); | ||
return BIR; | ||
} | ||
|
||
public boolean isBir() { | ||
return state.get(); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
package com.flipkart.gjex.core.web; | ||
|
||
import com.flipkart.gjex.core.task.RotationManagementBasedHealthCheck; | ||
|
||
import javax.inject.Inject; | ||
import javax.inject.Named; | ||
import javax.inject.Singleton; | ||
import javax.servlet.ServletContext; | ||
import javax.ws.rs.GET; | ||
import javax.ws.rs.Path; | ||
import javax.ws.rs.Produces; | ||
import javax.ws.rs.core.Context; | ||
import javax.ws.rs.core.MediaType; | ||
import javax.ws.rs.core.Response; | ||
|
||
/** | ||
* Resource for rotation management | ||
* @author ajay.jalgaonkar | ||
*/ | ||
|
||
@Singleton | ||
@Path("/") | ||
@Named | ||
public class RotationManagementResource { | ||
|
||
@Context | ||
private ServletContext servletContext; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not used . Can we please remove it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed |
||
|
||
@Inject | ||
public RotationManagementResource(RotationManagementBasedHealthCheck rotationManagementBasedHealthCheck) { | ||
this.rotationManagementBasedHealthCheck = rotationManagementBasedHealthCheck; | ||
} | ||
|
||
private RotationManagementBasedHealthCheck rotationManagementBasedHealthCheck; | ||
|
||
@GET | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ajay-jalgaonkar Lets not have this as GET's. A modification status should be an request which cannot be triggered by mistake or by automatic refresh. Lets move this to POST. Same with bir. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
@Path("/oor") | ||
@Produces(MediaType.APPLICATION_JSON) | ||
public Response oor() { | ||
String response = this.rotationManagementBasedHealthCheck.makeOor(); | ||
return Response.status(Response.Status.OK).entity(response).build(); | ||
} | ||
|
||
@GET | ||
@Produces(MediaType.APPLICATION_JSON) | ||
@Path("/bir") | ||
public Response bir() { | ||
String response = this.rotationManagementBasedHealthCheck.makeBir(); | ||
return Response.status(Response.Status.OK).entity(response).build(); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,26 +15,22 @@ | |
*/ | ||
package com.flipkart.gjex.examples.helloworld.guice; | ||
|
||
import org.glassfish.jersey.server.ResourceConfig; | ||
|
||
import io.dropwizard.metrics5.health.HealthCheck; | ||
import com.flipkart.gjex.core.filter.Filter; | ||
import com.flipkart.gjex.core.task.RotationManagementBasedHealthCheck; | ||
import com.flipkart.gjex.core.tracing.TracingSampler; | ||
import com.flipkart.gjex.examples.helloworld.filter.AuthFilter; | ||
import com.flipkart.gjex.examples.helloworld.filter.LoggingFilter; | ||
import com.flipkart.gjex.examples.helloworld.healthcheck.AllIsWellHealthCheck; | ||
import com.flipkart.gjex.examples.helloworld.service.GreeterService; | ||
import com.flipkart.gjex.examples.helloworld.tracing.AllWhitelistTracingSampler; | ||
import com.flipkart.gjex.examples.helloworld.web.HelloWorldResourceConfig; | ||
import com.google.inject.AbstractModule; | ||
import com.google.inject.name.Names; | ||
|
||
import io.dropwizard.metrics5.health.HealthCheck; | ||
import io.grpc.BindableService; | ||
import io.grpc.ManagedChannel; | ||
import io.grpc.ManagedChannelBuilder; | ||
import io.grpc.examples.helloworld.GreeterGrpc; | ||
|
||
|
||
import org.glassfish.jersey.server.ResourceConfig; | ||
|
||
/** | ||
* Guice module for wiring sample Service to GJEX runtime | ||
|
@@ -56,7 +52,7 @@ protected void configure() { | |
bind(Filter.class).annotatedWith(Names.named("LoggingFilter")).to(LoggingFilter.class); | ||
bind(Filter.class).annotatedWith(Names.named("AuthFilter")).to(AuthFilter.class); | ||
bind(TracingSampler.class).to(AllWhitelistTracingSampler.class); | ||
bind(HealthCheck.class).to(AllIsWellHealthCheck.class); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kingster Should we remove AllIsWellHealthCheck.java altogether considering we are providing a health check implementation by default as part of server? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can, but then users shouldn't need to bind this. This should be part of the defaults then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed AllIsWellHealthCheck There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be by default now. Lets change this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now binding of RotationManagementBasedHealthCheck is done in ApiModule |
||
bind(HealthCheck.class).to(RotationManagementBasedHealthCheck.class); | ||
bind(ResourceConfig.class).annotatedWith(Names.named("HelloWorldResourceConfig")).to(HelloWorldResourceConfig.class); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,12 +15,14 @@ | |
*/ | ||
package com.flipkart.gjex.examples.helloworld.web; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please revert this file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reverted |
||
|
||
import com.flipkart.gjex.core.config.RotationManagementConfig; | ||
import com.flipkart.gjex.core.web.RotationManagementResource; | ||
import org.glassfish.jersey.server.ResourceConfig; | ||
|
||
import javax.inject.Inject; | ||
import javax.inject.Named; | ||
import javax.inject.Singleton; | ||
|
||
import org.glassfish.jersey.server.ResourceConfig; | ||
|
||
/** | ||
* ResourceConfig example for registering all custom web resources for a GJEX application. | ||
* @author regu.b | ||
|
@@ -31,8 +33,8 @@ | |
public class HelloWorldResourceConfig extends ResourceConfig { | ||
|
||
@Inject | ||
public HelloWorldResourceConfig (HelloWorldResource1 helloWorldresource1, | ||
HelloWorldResource2 helloWorldresource2) { | ||
public HelloWorldResourceConfig (HelloWorldResource1 helloWorldresource1, | ||
HelloWorldResource2 helloWorldresource2) { | ||
register(helloWorldresource1); | ||
register(helloWorldresource2); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see any usage of this ResourceConfig. Can we please remove it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed