Skip to content

Commit

Permalink
[CELEBORN-1607] Enable useEnumCaseInsensitive for openapi-generator
Browse files Browse the repository at this point in the history
### What changes were proposed in this pull request?
Enable `useEnumCaseInsensitive` for openapi-generator.
And then in celeborn server end, the enum will be mapped to celeborn internal WorkerEventType.

### Why are the changes needed?

I met exception when sending worker event with openapi sdk.
```
Exception in thread "main" ApiException{code=400, responseHeaders={Server=[Jetty(9.4.52.v20230823)], Content-Length=[491], Date=[Fri, 20 Sep 2024 23:50:27 GMT], Content-Type=[text/plain]}, responseBody='Cannot deserialize value of type `org.apache.celeborn.rest.v1.model.SendWorkerEventRequest$EventTypeEnum` from String "DecommissionThenIdle": not one of the values accepted for Enum class: [DECOMMISSION_THEN_IDLE, GRACEFUL, NONE, DECOMMISSION, IMMEDIATELY, RECOMMISSION]
 at [Source: (org.glassfish.jersey.message.internal.ReaderInterceptorExecutor$UnCloseableInputStream); line: 1, column: 14] (through reference chain: org.apache.celeborn.rest.v1.model.SendWorkerEventRequest["eventType"])'}
    at org.apache.celeborn.rest.v1.master.invoker.ApiClient.processResponse(ApiClient.java:913)
    at org.apache.celeborn.rest.v1.master.invoker.ApiClient.invokeAPI(ApiClient.java:1000)
    at org.apache.celeborn.rest.v1.master.WorkerApi.sendWorkerEvent(WorkerApi.java:378)
    at org.apache.celeborn.rest.v1.master.WorkerApi.sendWorkerEvent(WorkerApi.java:334)
    at org.example.Main.main(Main.java:22)

```

The testing code to re-produce:
```
package org.example;

import org.apache.celeborn.rest.v1.master.WorkerApi;
import org.apache.celeborn.rest.v1.master.invoker.ApiClient;
import org.apache.celeborn.rest.v1.model.ExcludeWorkerRequest;
import org.apache.celeborn.rest.v1.model.SendWorkerEventRequest;
import org.apache.celeborn.rest.v1.model.WorkerId;

public class Main {
    public static void main(String[] args) throws Exception {

        String cmUrl = "http://localhost:9098";
        WorkerApi workerApi = new WorkerApi(new ApiClient().setBasePath(cmUrl));
        workerApi.excludeWorker(new ExcludeWorkerRequest()
                .addAddItem(new WorkerId()
                        .host("localhost")
                        .rpcPort(1)
                        .pushPort(2)
                        .fetchPort(3)
                        .replicatePort(4)));
        workerApi.sendWorkerEvent(new SendWorkerEventRequest()
                        .addWorkersItem(new WorkerId()
                                .host("127.0.0.1")
                                .rpcPort(56116)
                                .pushPort(56117)
                                .fetchPort(56119)
                                .replicatePort(56118))
                .eventType(SendWorkerEventRequest.EventTypeEnum.DECOMMISSION_THEN_IDLE));
    }
}
```

Seems because for the EventTypeEnum, the name and value not the same and then cause this issue.

Not sure why the UT passed, but the integration testing failed.

For EventTypeEnum, because its value is case sensitive, so we meet this issue.

https://github.com/apache/celeborn/blob/8734d1663884345477257d2f10f4c19732c6f1c3/openapi/openapi-client/src/main/java/org/apache/celeborn/rest/v1/model/SendWorkerEventRequest.java#L47-L83

Related issue in jersey end I think, eclipse-ee4j/jersey#5288

In this PR, `useEnumCaseInsensitive` is enabled for openapi-generator.

### Does this PR introduce _any_ user-facing change?
No, there is not user facing change and this SDK has not been released yet.

### How was this patch tested?
Existing UT and Integration testing.
<img width="1265" alt="image" src="https://github.com/user-attachments/assets/6a34a0dd-c474-4e8d-b372-19b0fda94972">

Closes #2754 from turboFei/eventTypeEnumMapping.

Authored-by: Wang, Fei <[email protected]>
Signed-off-by: mingji <[email protected]>
  • Loading branch information
turboFei authored and FMX committed Sep 23, 2024
1 parent 6e07134 commit 1596cff
Show file tree
Hide file tree
Showing 15 changed files with 58 additions and 35 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -425,11 +425,11 @@ object ControlMessages extends Logging {
object WorkerEventRequest {
def apply(
workers: util.List[WorkerInfo],
eventType: String,
eventType: WorkerEventType,
requestId: String): PbWorkerEventRequest =
PbWorkerEventRequest.newBuilder()
.setRequestId(requestId)
.setWorkerEventType(WorkerEventType.valueOf(eventType))
.setWorkerEventType(eventType)
.addAllWorkers(workers.asScala.map { workerInfo =>
PbSerDeUtils.toPbWorkerInfo(workerInfo, true, false)
}.toList.asJava)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1218,7 +1218,7 @@ private[celeborn] class Master(
}

override def handleWorkerEvent(
workerEventType: String,
workerEventType: WorkerEventType,
workers: Seq[WorkerInfo]): HandleResponse = {
val sb = new StringBuilder()
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import io.swagger.v3.oas.annotations.tags.Tag
import org.apache.commons.lang3.StringUtils

import org.apache.celeborn.common.meta.WorkerInfo
import org.apache.celeborn.common.protocol.WorkerEventType
import org.apache.celeborn.server.common.http.api.ApiRequestContext

@Tag(name = "Deprecated")
Expand Down Expand Up @@ -136,7 +137,9 @@ class ApiMasterResource extends ApiRequestContext {
}
sb.append("============================ Handle Worker Event =============================\n")
val workerList = workers.split(",").filter(_.nonEmpty).map(WorkerInfo.fromUniqueId)
sb.append(httpService.handleWorkerEvent(normalizeParam(eventType), workerList)._2)
sb.append(httpService.handleWorkerEvent(
WorkerEventType.valueOf(normalizeParam(eventType)),
workerList)._2)
sb.toString()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ import io.swagger.v3.oas.annotations.media.{Content, Schema}
import io.swagger.v3.oas.annotations.responses.ApiResponse
import io.swagger.v3.oas.annotations.tags.Tag

import org.apache.celeborn.common.protocol.WorkerEventType
import org.apache.celeborn.rest.v1.model._
import org.apache.celeborn.rest.v1.model.SendWorkerEventRequest.EventTypeEnum
import org.apache.celeborn.server.common.http.api.ApiRequestContext
import org.apache.celeborn.server.common.http.api.v1.ApiUtils
import org.apache.celeborn.service.deploy.master.Master
Expand Down Expand Up @@ -137,7 +139,8 @@ class WorkerResource extends ApiRequestContext {
throw new BadRequestException(
s"None of the workers are known: ${unknownWorkers.map(_.readableAddress).mkString(", ")}")
}
val (success, msg) = httpService.handleWorkerEvent(request.getEventType.toString, workers)
val (success, msg) =
httpService.handleWorkerEvent(toWorkerEventType(request.getEventType), workers)
val finalMsg =
if (unknownWorkers.isEmpty) {
msg
Expand All @@ -146,4 +149,16 @@ class WorkerResource extends ApiRequestContext {
}
new HandleResponse().success(success).message(finalMsg)
}

private def toWorkerEventType(enum: EventTypeEnum): WorkerEventType = {
enum match {
case EventTypeEnum.NONE => WorkerEventType.None
case EventTypeEnum.IMMEDIATELY => WorkerEventType.Immediately
case EventTypeEnum.DECOMMISSION => WorkerEventType.Decommission
case EventTypeEnum.DECOMMISSIONTHENIDLE => WorkerEventType.DecommissionThenIdle
case EventTypeEnum.GRACEFUL => WorkerEventType.Graceful
case EventTypeEnum.RECOMMISSION => WorkerEventType.Recommission
case _ => WorkerEventType.UNRECOGNIZED
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ class ApiV1MasterResourceSuite extends ApiV1BaseResourceSuite {
Entity.entity(sendWorkerEventRequest, MediaType.APPLICATION_JSON))
assert(HttpServletResponse.SC_BAD_REQUEST == response.getStatus)
assert(
response.readEntity(classOf[String]).contains("eventType(None) and workers([]) are required"))
response.readEntity(classOf[String]).contains("eventType(NONE) and workers([]) are required"))
sendWorkerEventRequest.workers(Collections.singletonList(worker))
response = webTarget.path("workers/events").request(MediaType.APPLICATION_JSON).post(
Entity.entity(sendWorkerEventRequest, MediaType.APPLICATION_JSON))
Expand Down
2 changes: 2 additions & 0 deletions openapi/openapi-client/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@
<hideGenerationTimestamp>true</hideGenerationTimestamp>
<supportUrlQuery>false</supportUrlQuery>
<annotationLibrary>none</annotationLibrary>
<useEnumCaseInsensitive>true</useEnumCaseInsensitive>
</configOptions>
</configuration>
</execution>
Expand Down Expand Up @@ -278,6 +279,7 @@
<hideGenerationTimestamp>true</hideGenerationTimestamp>
<supportUrlQuery>false</supportUrlQuery>
<annotationLibrary>none</annotationLibrary>
<useEnumCaseInsensitive>true</useEnumCaseInsensitive>
</configOptions>
</configuration>
</execution>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public String toString() {
@JsonCreator
public static LevelEnum fromValue(String value) {
for (LevelEnum b : LevelEnum.values()) {
if (b.value.equals(value)) {
if (b.value.equalsIgnoreCase(value)) {
return b;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public String toString() {
@JsonCreator
public static ModeEnum fromValue(String value) {
for (ModeEnum b : ModeEnum.values()) {
if (b.value.equals(value)) {
if (b.value.equalsIgnoreCase(value)) {
return b;
}
}
Expand Down Expand Up @@ -123,7 +123,7 @@ public String toString() {
@JsonCreator
public static StorageEnum fromValue(String value) {
for (StorageEnum b : StorageEnum.values()) {
if (b.value.equals(value)) {
if (b.value.equalsIgnoreCase(value)) {
return b;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,17 @@ public class SendWorkerEventRequest {
* The type of the event.
*/
public enum EventTypeEnum {
IMMEDIATELY("Immediately"),
IMMEDIATELY("IMMEDIATELY"),

DECOMMISSION("Decommission"),
DECOMMISSION("DECOMMISSION"),

DECOMMISSION_THEN_IDLE("DecommissionThenIdle"),
DECOMMISSIONTHENIDLE("DECOMMISSIONTHENIDLE"),

GRACEFUL("Graceful"),
GRACEFUL("GRACEFUL"),

RECOMMISSION("Recommission"),
RECOMMISSION("RECOMMISSION"),

NONE("None");
NONE("NONE");

private String value;

Expand All @@ -76,7 +76,7 @@ public String toString() {
@JsonCreator
public static EventTypeEnum fromValue(String value) {
for (EventTypeEnum b : EventTypeEnum.values()) {
if (b.value.equals(value)) {
if (b.value.equalsIgnoreCase(value)) {
return b;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,13 @@ public class WorkerExitRequest {
* The type of the worker exit request.
*/
public enum TypeEnum {
DECOMMISSION("Decommission"),
DECOMMISSION("DECOMMISSION"),

GRACEFUL("Graceful"),
GRACEFUL("GRACEFUL"),

IMMEDIATELY("Immediately"),
IMMEDIATELY("IMMEDIATELY"),

NONE("None");
NONE("NONE");

private String value;

Expand All @@ -67,7 +67,7 @@ public String toString() {
@JsonCreator
public static TypeEnum fromValue(String value) {
for (TypeEnum b : TypeEnum.values()) {
if (b.value.equals(value)) {
if (b.value.equalsIgnoreCase(value)) {
return b;
}
}
Expand Down
12 changes: 6 additions & 6 deletions openapi/openapi-client/src/main/openapi3/master_rest_v1.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -706,12 +706,12 @@ components:
type: string
description: The type of the event.
enum:
- Immediately
- Decommission
- DecommissionThenIdle
- Graceful
- Recommission
- None
- IMMEDIATELY
- DECOMMISSION
- DECOMMISSIONTHENIDLE
- GRACEFUL
- RECOMMISSION
- NONE
workers:
type: array
description: The workers to send the event.
Expand Down
10 changes: 5 additions & 5 deletions openapi/openapi-client/src/main/openapi3/worker_rest_v1.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -602,13 +602,13 @@ components:
properties:
type:
type: string
default: None
default: NONE
description: The type of the worker exit request.
enum:
- Decommission
- Graceful
- Immediately
- None
- DECOMMISSION
- GRACEFUL
- IMMEDIATELY
- NONE

HandleResponse:
type: object
Expand Down
1 change: 1 addition & 0 deletions project/CelebornBuild.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1364,6 +1364,7 @@ object CelebornOpenApi {
"supportUrlQuery" -> "false",
"annotationLibrary" -> "none",
"templateDir" -> s"$openApiSpecDir/templates",
"useEnumCaseInsensitive" -> "true"
)
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import org.eclipse.jetty.servlet.FilterHolder
import org.apache.celeborn.common.CelebornConf
import org.apache.celeborn.common.internal.Logging
import org.apache.celeborn.common.meta.WorkerInfo
import org.apache.celeborn.common.protocol.TransportModuleConstants
import org.apache.celeborn.common.protocol.{TransportModuleConstants, WorkerEventType}
import org.apache.celeborn.common.util.Utils
import org.apache.celeborn.server.common.http.HttpServer
import org.apache.celeborn.server.common.http.api.ApiRootResource
Expand Down Expand Up @@ -187,7 +187,9 @@ abstract class HttpService extends Service with Logging {

def exit(exitType: String): String = throw new UnsupportedOperationException()

def handleWorkerEvent(workerEventType: String, workers: Seq[WorkerInfo]): HandleResponse =
def handleWorkerEvent(
workerEventType: WorkerEventType,
workers: Seq[WorkerInfo]): HandleResponse =
throw new UnsupportedOperationException()

def getWorkerEventInfo(): String = throw new UnsupportedOperationException()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ class ApiV1OpenapiClientSuite extends ApiV1WorkerOpenapiClientSuite {

handleResponse = api.sendWorkerEvent(
new SendWorkerEventRequest().addWorkersItem(workerId).eventType(
EventTypeEnum.DECOMMISSION_THEN_IDLE))
EventTypeEnum.DECOMMISSIONTHENIDLE))
assert(handleResponse.getSuccess)

assert(!api.getWorkerEvents.getWorkerEvents.isEmpty)
Expand Down

0 comments on commit 1596cff

Please sign in to comment.