kfaraz commented on a change in pull request #12026:
URL: https://github.com/apache/druid/pull/12026#discussion_r762712632
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunner.java
##########
@@ -1637,30 +1631,23 @@ public Response setEndOffsets(
resume();
return Response.ok(sequenceNumbers).build();
} else if (latestSequence.isCheckpointed()) {
- return Response.status(Response.Status.BAD_REQUEST)
- .entity(StringUtils.format(
+ return
ResponseStatusExceptionMapper.toResponse(Response.Status.BAD_REQUEST,
Review comment:
_Nit: style choice_
Maybe break the line after `.toResponse(`, would look more readable if the
arguments start from a new line.
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java
##########
@@ -569,9 +557,7 @@ public Response getTasks(
//check for valid state
if (state != null) {
if (!API_TASK_STATES.contains(StringUtils.toLowerCase(state))) {
- return Response.status(Status.BAD_REQUEST)
- .entity(StringUtils.format("Invalid state : %s, valid
values are: %s", state, API_TASK_STATES))
- .build();
+ throw new BadRequestException(StringUtils.format("Invalid state : %s,
valid values are: %s", state, API_TASK_STATES));
Review comment:
Same comment of change from returning a `Response` to throwing an
exception.
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunner.java
##########
@@ -1637,30 +1631,23 @@ public Response setEndOffsets(
resume();
return Response.ok(sequenceNumbers).build();
} else if (latestSequence.isCheckpointed()) {
- return Response.status(Response.Status.BAD_REQUEST)
- .entity(StringUtils.format(
+ return
ResponseStatusExceptionMapper.toResponse(Response.Status.BAD_REQUEST,
+ StringUtils.format(
"Sequence [%s] has already endOffsets set, cannot
set to [%s]",
latestSequence,
- sequenceNumbers
- )).build();
+ sequenceNumbers));
} else if (!isPaused()) {
- return Response.status(Response.Status.BAD_REQUEST)
- .entity("Task must be paused before changing the end
offsets")
- .build();
+ return
ResponseStatusExceptionMapper.toResponse(Response.Status.BAD_REQUEST,
+ "Task must be paused
before changing the end offsets");
}
for (Map.Entry<PartitionIdType, SequenceOffsetType> entry :
sequenceNumbers.entrySet()) {
if
(createSequenceNumber(entry.getValue()).compareTo(createSequenceNumber(currOffsets.get(entry.getKey())))
< 0) {
- return Response.status(Response.Status.BAD_REQUEST)
- .entity(
- StringUtils.format(
- "End sequence must be >= current sequence
for partition [%s] (current: %s)",
- entry.getKey(),
- currOffsets.get(entry.getKey())
- )
- )
- .build();
+ return
ResponseStatusExceptionMapper.toResponse(Response.Status.BAD_REQUEST,
Review comment:
_Nit: same format suggestion as above_
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java
##########
@@ -177,25 +179,13 @@ public Response taskPost(final Task task, @Context final
HttpServletRequest req)
return asLeaderWith(
taskMaster.getTaskQueue(),
- new Function<TaskQueue, Response>()
- {
- @Override
- public Response apply(TaskQueue taskQueue)
- {
- try {
- taskQueue.add(task);
- return Response.ok(ImmutableMap.of("task",
task.getId())).build();
- }
- catch (EntryExistsException e) {
- return Response.status(Response.Status.BAD_REQUEST)
- .entity(
- ImmutableMap.of(
- "error",
- StringUtils.format("Task[%s] already
exists!", task.getId())
- )
- )
- .build();
- }
+ taskQueue -> {
+ try {
+ taskQueue.add(task);
+ return Response.ok(ImmutableMap.of("task", task.getId())).build();
+ }
+ catch (EntryExistsException e) {
+ throw new BadRequestException(StringUtils.format("Task[%s] already
exists!", task.getId()));
Review comment:
Earlier, this was returning a response like `return
Response.status(BAD_REQUEST).entity().build()`. Shouldn't we continue to return
a `Response` here instead of throwing an exception?
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java
##########
@@ -468,9 +458,7 @@ public Response getWorkerConfigHistory(
return Response.ok(workerEntryList).build();
}
catch (IllegalArgumentException e) {
- return Response.status(Response.Status.BAD_REQUEST)
- .entity(ImmutableMap.<String, Object>of("error",
e.getMessage()))
- .build();
+ throw new BadRequestException(e.getMessage());
Review comment:
Same comment of change from returning a `Response` to throwing an
exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
