epugh commented on PR #4452:
URL: https://github.com/apache/solr/pull/4452#issuecomment-4762681923
@jaykay12 I got some AI help.... And there are some things that we need
to improve. I'm going to try and summarize here, and then share the AI report
below. I've gone through and edited/confirmed what the AI flagged!
1) For the V1 API, we need to return the old format `"id: 25, status:
inactive"` for the status, even though it's terrible, so that we don't
introduce a breaking change! I think I actually advocated with you to deal
wiht it in v1, but now I am realizing we can't ;-(.
2) In the V1 approach we collected tasks from all shards by fanning out the
checks, but we have dropped that in this PR. "Distributed regression —
cross-shard task aggregation is silently dropped" is what is in the AI report
below. It turns out that we never had a unit test that covered this situation,
so I think we need to start with that.
Critical Issues
**1. Distributed regression — cross-shard task aggregation is silently
dropped**
The deleted `ActiveTasksListComponent` had a `handleResponses()` method that
aggregated task results across all shards in a distributed query. The new
`ActiveTask` only queries the local core:
```java
// Only checks this shard's CancellableQueryTracker
solrQueryRequest.getCore().getCancellableQueryTracker().isQueryIdActive(taskID)
```
In a distributed collection, a task running on shard 2 will be invisible to
a client hitting shard 1's coordinator. The new implementation silently drops
this cross-shard fan-out.
**2. `TaskStatus` enum missing `@JsonValue` — serializes as uppercase**
```java
public enum TaskStatus {
ACTIVE("active"),
INACTIVE("inactive");
private final String value;
public String getValue() { return this.value; }
}
```
Without `@JsonValue` on `getValue()`, Jackson serializes by enum constant
name: `"ACTIVE"` / `"INACTIVE"`. The `value` field is unused at the wire level
unless `@JsonValue` is added:
```java
@JsonValue
public String getValue() { return this.value; }
```
**3. V1 response format breaking change**
Old `ActiveTasksListComponent.handleResponses()` returned a formatted string:
```
"id: 5, status: active"
```
New `ActiveTasksListHandler.handleRequestBody()` returns a boolean:
```java
boolean taskStatus = taskStatusResponse.taskStatus.equals(ACTIVE);
rsp.add("taskStatus", taskStatus); // true/false, not a string
```
This breaks existing v1 clients that parse the old string format. The new
integration tests also cast to `String` and call `.contains("active")` — if
this is actually returning a boolean, those tests would throw
`ClassCastException`.
---
### Design Issues
**4. New tests may not be exercising the task status path**
`testCheckSpecificQueryStatus_Active` and
`testCheckSpecificQueryStatus_Inactive` set `params.set("taskUUID", "5")`, but
`ActiveTasksListHandler` checks `req.getParams().get(TASK_CHECK_UUID, null)`
where `TASK_CHECK_UUID` comes from `CommonParams`. Verify these are the same
parameter name — if not, the tests fall through to the list-all-tasks path and
`queryResponse.get("taskStatus")` will be `null`, not `"active"`/`"inactive"`.
**5. `@JsonProperty` names are camelCase, not kebab-case**
Solr's JAX-RS model convention uses kebab-case property names (e.g.
`"task-id"`, `"task-query"`). The new models use camelCase:
```java
@JsonProperty public String taskID; // "taskID" on the wire
@JsonProperty public String taskQuery; // "taskQuery" on the wire
```
Compare to e.g. `SchemaDesignerResponse` which uses
`@JsonProperty("configSet")` with explicit strings. These should at minimum
have explicit `@JsonProperty("taskID")` to make the contract clear, or be
changed to `@JsonProperty("task-id")` to follow convention.
---
### Minor Issues
**7. Unnecessary no-arg constructor in `ActiveTaskDetails`**
```java
public ActiveTaskDetails() {} // Jackson doesn't need this — fields are
public
```
Remove it; the parameterized constructor is sufficient for test/internal
use, and Jackson can access public fields directly.
**8. Unrelated change in `HealthCheckHandler.java`**
The removal of the Javadoc line about `NodeHealth` delegation appears
unrelated to this PR. Worth either moving to a separate commit/PR or explaining
why it's included here.
**9. `TasksApi` interface — `@StoreApiParameters` on a `@GET` endpoint**
Verify that `@StoreApiParameters` is appropriate for these read-only
endpoints. It's used on both methods in `TasksApi` — check whether this
annotation has side effects that are undesirable for a pure-read listing
operation.
---
### Test Coverage
- `ActiveTaskTest.java` covers the happy path for both methods with mocked
deps — good baseline.
- Missing: test for `listAllActiveTasks()` when no tasks are active (empty
iterator).
- Missing: test verifying the v1 `handleRequestBody` path actually returns
the right format.
- The new `TestTaskManagement` tests (items 3 and 4 above) need review to
confirm they're hitting the intended code paths.
---
### Summary Table
| Issue | Severity |
|---|---|
| Distributed cross-shard aggregation removed | High |
| `@JsonValue` missing on `TaskStatus` enum | High |
| V1 boolean vs. string response format break | High |
| New integration tests may use wrong param name | High |
| `@JsonProperty` names — camelCase vs. convention | Medium |
| Unrelated `HealthCheckHandler` Javadoc change | Low |
| Unnecessary `ActiveTaskDetails()` constructor | Low
--
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]
