Added Rotation Management #63

ajaypj · 2024-03-20T13:19:56Z

No description provided.

kingster · 2024-03-22T05:50:58Z

core/src/main/java/com/flipkart/gjex/core/web/RotationManagementResource.java

+  @GET
+  @Produces(MediaType.APPLICATION_JSON)
+  @Path("/status")
+  public Response status() {
+    String response = this.manualRotationManagement.getStatus();
+    if (this.manualRotationManagement.inHealthy()) {
+      return Response.status(Response.Status.OK).entity(response).build();
+    }
+    return Response.status(Response.Status.NOT_FOUND).entity(response).build();
+  }


Is this really required? Wouldn't the main heathcheck API simply provide the same?

kingster · 2024-03-22T05:53:07Z

...s/src/main/java/com/flipkart/gjex/examples/helloworld/web/HelloWorldHealthCheckResource.java

+public class HelloWorldHealthCheckResource {
+  private ManualRotationManagement manualRotationManagement;
+
+  @Inject
+  public HelloWorldHealthCheckResource(ManualRotationManagement manualRotationManagement) {
+    this.manualRotationManagement = manualRotationManagement;
+  }
+
+  @GET
+  @Produces(MediaType.APPLICATION_JSON)
+  @Path("/")
+  public Response health() {
+    if (manualRotationManagement.inHealthy()) {
+       return Response.status(Response.Status.OK).build();
+    } else {
+      return Response.status(Response.Status.SERVICE_UNAVAILABLE).build();
+    }
+  }
+}


Why is this resource required? Wouldn't the HealthCheckResource itself provide this?

This is not required because this functionality is handled by HealthCheckResource. Deleted.

kingster · 2024-03-22T05:54:17Z

examples/src/main/java/com/flipkart/gjex/examples/helloworld/web/HelloWorldResourceConfig.java

-			HelloWorldResource2 helloWorldresource2) {
+	public HelloWorldResourceConfig (HelloWorldResource1 helloWorldresource1,
+																	 HelloWorldResource2 helloWorldresource2,
+																	 RotationManagementResource rotationManagementResource,


RotationManagementResource should be by default added, and not required to be explicitly added. Lets change the behavior.

added by default

added RotationManagementConfig and removed binding of RotationManagementResource

spawn92 · 2024-03-27T03:58:34Z

examples/src/main/java/com/flipkart/gjex/examples/helloworld/web/HelloWorldResourceConfig.java

 		register(helloWorldresource1);
 		register(helloWorldresource2);
+		register(rotationManagementResource);


This should not be required. We are registering rotationManagement as part of setting up jetty server

spawn92 · 2024-03-27T04:03:29Z

guice/src/main/java/com/flipkart/gjex/guice/module/DashboardModule.java

+		rotationManagementResourceConfig.register(provider);
+		ServletHolder rotationManagementServlet =
+				new ServletHolder(new ServletContainer(rotationManagementResourceConfig));
+		context.addServlet(rotationManagementServlet, "/rotation"); // registering Rotation


You will have to use wildcard here if you want to register multiple API paths to this servlet .
context.addServlet(rotationManagementServlet, "/rotation*");

updated path as /rotation/*

…eConfig

spawn92 · 2024-03-27T14:03:10Z

core/src/main/java/com/flipkart/gjex/core/config/RotationManagementConfig.java

@@ -0,0 +1,24 @@
+package com.flipkart.gjex.core.config;


I do not see any usage of this ResourceConfig. Can we please remove it ?

spawn92 · 2024-03-27T14:04:24Z

core/src/main/java/com/flipkart/gjex/core/task/RotationManagementBasedHealthCheck.java

+  private static final String BIR = "bir";
+  private static final String OOR = "oor";
+
+  private AtomicBoolean state;


Can we refactor state attribute to maybe inRotation to signify what it will actually store .

refactored to rotationStatus

spawn92 · 2024-03-27T14:04:58Z

core/src/main/java/com/flipkart/gjex/core/task/RotationManagementBasedHealthCheck.java

+  @Override
+  protected Result check() {
+    if (isBir()) {
+      info("Returning healthy status.");


I feel these info logs are not necessary for every health check request.

spawn92 · 2024-03-27T14:05:21Z

core/src/main/java/com/flipkart/gjex/core/web/RotationManagementResource.java

+public class RotationManagementResource {
+
+  @Context
+  private ServletContext servletContext;


This is not used . Can we please remove it

spawn92 · 2024-03-27T14:06:38Z

examples/src/main/java/com/flipkart/gjex/examples/helloworld/guice/HelloWorldModule.java

@@ -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);


@kingster Should we remove AllIsWellHealthCheck.java altogether considering we are providing a health check implementation by default as part of server?

We can, but then users shouldn't need to bind this. This should be part of the defaults then.

removed AllIsWellHealthCheck

This should be by default now. Lets change this

Now binding of RotationManagementBasedHealthCheck is done in ApiModule

spawn92 · 2024-03-27T14:06:50Z

examples/src/main/java/com/flipkart/gjex/examples/helloworld/web/HelloWorldResourceConfig.java

@@ -15,12 +15,14 @@
 */
 package com.flipkart.gjex.examples.helloworld.web;


Please revert this file

spawn92 · 2024-03-27T14:14:55Z

core/src/main/java/com/flipkart/gjex/core/task/RotationManagementBasedHealthCheck.java

@@ -0,0 +1,54 @@
+package com.flipkart.gjex.core.task;


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 .

spawn92 · 2024-03-28T08:47:27Z

core/src/main/java/com/flipkart/gjex/core/task/RotationManagementBasedHealthCheck.java

+  private static final String BIR = "bir";
+  private static final String OOR = "oor";
+
+  private AtomicBoolean state;


Please make this AtomicBoolean a final as well . We do not require more than 1 instance of this attribute

core/src/main/java/com/flipkart/gjex/core/healthcheck/RotationManagementBasedHealthCheck.java

spawn92

lgtm

kingster · 2024-04-01T17:23:21Z

core/src/main/java/com/flipkart/gjex/core/web/RotationManagementResource.java

+
+  private RotationManagementBasedHealthCheck rotationManagementBasedHealthCheck;
+
+  @GET


@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.

kingster · 2024-04-01T17:23:46Z

core/src/main/java/com/flipkart/gjex/core/healthcheck/RotationManagementBasedHealthCheck.java

+    return BIR;
+  }
+
+  public boolean isBir() {


Can we rename this method to inRotation

* master: Added Rotation Management (#63) Create clojars publish pipeline (#65) Create LICENSE (#62) Convert LoggingFilter to Generic<Req, Res> Filter (#61) Update version for current iteration 1.40-SNAPSHOT Make mvn grpc-jexpress-template upto date with Java 17/21 Support (#60) Support for proto-reflections for swagger/postman (#59) Update README.md gRPC & ProtoC Version Bump (#57) Bump fasterxml.jackson version (#58)

ajay-jalgaonkar added 5 commits March 20, 2024 13:04

Added rotationmanagement and removed unused imports

29eb85d

Added rotationmanagement and removed unused imports

a0ffdd8

Added rotationmanagement and removed unused imports

e88fac0

uncommenting greeter service

96e0f9a

resetting only import related files

282db60

kingster requested changes Mar 22, 2024

View reviewed changes

Removed HelloWorldHealthCheckResource.java,

c8ce346

added RotationManagementConfig and removed binding of RotationManagementResource

ajaypj force-pushed the rotation-mgmt branch from 53f9e69 to c8ce346 Compare March 22, 2024 07:24

rotation changes

e549aa3

spawn92 reviewed Mar 27, 2024

View reviewed changes

ajay-jalgaonkar added 2 commits March 27, 2024 12:41

updated path for rotation and removed rotation from HelloWorldResourc…

bb6cfd4

…eConfig

removed unnecessary changes and imports

5816338

spawn92 reviewed Mar 27, 2024

View reviewed changes

spawn92 reviewed Mar 28, 2024

View reviewed changes

moved healthcheck files and removed RotationManagementConfig.java

6d74d24

spawn92 reviewed Apr 1, 2024

View reviewed changes

core/src/main/java/com/flipkart/gjex/core/healthcheck/RotationManagementBasedHealthCheck.java Show resolved Hide resolved

binding RotationManagementBasedHealthCheck in ApiModule

2bb44b6

spawn92 previously approved these changes Apr 1, 2024

View reviewed changes

kingster reviewed Apr 1, 2024

View reviewed changes

ajaypj and others added 2 commits April 1, 2024 22:54

Merge branch 'master' into rotation-mgmt

b7fb378

renamed isBir function and made oor and bir calls POST requests

ec9e44e

ajaypj dismissed spawn92’s stale review via ec9e44e April 1, 2024 17:29

kingster approved these changes Apr 1, 2024

View reviewed changes

kingster merged commit ed963fa into flipkart-incubator:master Apr 1, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Rotation Management #63

Added Rotation Management #63

ajaypj commented Mar 20, 2024

kingster Mar 22, 2024

ajaypj Mar 29, 2024

kingster Mar 22, 2024

ajaypj Mar 22, 2024

kingster Mar 22, 2024

ajaypj Mar 29, 2024

spawn92 Mar 27, 2024

ajaypj Mar 27, 2024

spawn92 Mar 27, 2024 •

edited

Loading

ajaypj Mar 27, 2024

spawn92 Mar 27, 2024

ajaypj Mar 29, 2024

spawn92 Mar 27, 2024

ajaypj Mar 29, 2024

spawn92 Mar 27, 2024

ajaypj Mar 29, 2024

spawn92 Mar 27, 2024

ajaypj Mar 29, 2024

spawn92 Mar 27, 2024

kingster Mar 29, 2024

ajaypj Mar 29, 2024

kingster Apr 1, 2024 •

edited

Loading

ajaypj Apr 1, 2024

spawn92 Mar 27, 2024

ajaypj Mar 29, 2024

spawn92 Mar 27, 2024

ajaypj Mar 29, 2024

spawn92 Mar 28, 2024

ajaypj Mar 29, 2024

spawn92 left a comment

kingster Apr 1, 2024

ajaypj Apr 1, 2024

kingster Apr 1, 2024

ajaypj Apr 1, 2024

		@@ -15,12 +15,14 @@
		*/
		package com.flipkart.gjex.examples.helloworld.web;


		private RotationManagementBasedHealthCheck rotationManagementBasedHealthCheck;

		@GET

Added Rotation Management #63

Added Rotation Management #63

Conversation

ajaypj commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spawn92 Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingster Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spawn92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spawn92 Mar 27, 2024 •

edited

Loading

kingster Apr 1, 2024 •

edited

Loading