kfaraz commented on code in PR #17723:
URL: https://github.com/apache/druid/pull/17723#discussion_r1957144967
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunner.java:
##########
@@ -1887,10 +1887,12 @@ public Response pauseHTTP(
@VisibleForTesting
public Response pause() throws InterruptedException
{
- if (!(status == Status.PAUSED || status == Status.READING)) {
+ Status currentStatus = status;
Review Comment:
```suggestion
// Read the volatile status into a variable so that its value does not
change while the condition is evaluated
final Status currentStatus = status;
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunner.java:
##########
@@ -1887,10 +1887,12 @@ public Response pauseHTTP(
@VisibleForTesting
public Response pause() throws InterruptedException
{
- if (!(status == Status.PAUSED || status == Status.READING)) {
+ Status currentStatus = status;
+ if (!(currentStatus == Status.PAUSED || currentStatus == Status.READING)) {
+ log.debug("Returning 409 conflict for task [%s] with currentState: [%s],
volatileState: [%s]", task.getId(), currentStatus, status);
return Response.status(Response.Status.CONFLICT)
.type(MediaType.TEXT_PLAIN)
- .entity(StringUtils.format("Can't pause, task is not in a
pausable state (state: [%s])", status))
+ .entity(StringUtils.format("Can't pause, task is not in a
pausable state (currentState: [%s], volatileState: [%s])", currentStatus,
status))
Review Comment:
Simplified the error message a bit.
We need not include the volatile status in the error message as it is not
used in the if block.
```suggestion
.entity(StringUtils.format("Cannot pause task as it is
currently in state[%s]", currentStatus))
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunner.java:
##########
@@ -1887,10 +1887,12 @@ public Response pauseHTTP(
@VisibleForTesting
public Response pause() throws InterruptedException
{
- if (!(status == Status.PAUSED || status == Status.READING)) {
+ Status currentStatus = status;
+ if (!(currentStatus == Status.PAUSED || currentStatus == Status.READING)) {
+ log.debug("Returning 409 conflict for task [%s] with currentState: [%s],
volatileState: [%s]", task.getId(), currentStatus, status);
Review Comment:
Let's make this an error log for now.
```suggestion
log.error("Cannot pause task[%s] as it is currently in state[%s]",
task.getId(), currentStatus);
```
--
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]
