kfaraz commented on code in PR #16439:
URL: https://github.com/apache/druid/pull/16439#discussion_r1600273970
##########
server/src/main/java/org/apache/druid/server/coordination/SegmentChangeStatus.java:
##########
@@ -71,6 +75,25 @@ public String getFailureCause()
return failureCause;
}
+ @Override
+ public boolean equals(Object o)
+ {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ SegmentChangeStatus that = (SegmentChangeStatus) o;
+ return state == that.state && Objects.equals(failureCause,
that.failureCause);
+ }
+
+ @Override
+ public int hashCode()
+ {
+ return Objects.hash(state, failureCause);
+ }
Review Comment:
I would generally prefer to add `equals`/`hashCode` when it is needed for
the actual implementation or if the class has several fields and it makes
testing easier.
Since we currently have only 2 fields in this class, we can probably skip it
for the time being. Especially since there is only one test which is using this.
##########
server/src/main/java/org/apache/druid/server/coordination/SegmentChangeStatus.java:
##########
@@ -54,7 +55,10 @@ private SegmentChangeStatus(
@JsonProperty("failureCause") @Nullable String failureCause
)
{
- this.state = Preconditions.checkNotNull(state, "state must be non-null");
+ if (state == null) {
+ throw DruidException.defensive("state must be non-null");
Review Comment:
```suggestion
throw DruidException.defensive("SegmentChangeStatus must have a
non-null state");
```
##########
server/src/test/java/org/apache/druid/server/coordination/SegmentLoadDropHandlerTest.java:
##########
@@ -567,6 +568,48 @@ public void
testProcessBatchDuplicateLoadRequestsWhenFirstRequestFailsSecondRequ
segmentLoadDropHandler.stop();
}
+ @Test(timeout = 60_000L)
+ public void testProcessBatchWithDefaultNoStorageLocations() throws Exception
Review Comment:
```suggestion
public void testProcessBatchWithEmptyStorageLocations() throws Exception
```
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLoaderConfig.java:
##########
@@ -120,7 +121,14 @@ public int
getNumThreadsToLoadSegmentsIntoPageCacheOnBootstrap()
public File getInfoDir()
{
if (infoDir == null) {
- infoDir = new File(locations.get(0).getPath(), "info_dir");
+ if (locations.isEmpty()) {
+ throw DruidException.forPersona(DruidException.Persona.OPERATOR)
+ .ofCategory(DruidException.Category.NOT_FOUND)
+ .build("storage locations are empty. At least one storage path
must be specified "
Review Comment:
```suggestion
.build("Storage locations are empty. At least one storage path
must be specified "
```
##########
server/src/test/java/org/apache/druid/server/coordination/SegmentLoadDropHandlerTest.java:
##########
@@ -567,6 +568,48 @@ public void
testProcessBatchDuplicateLoadRequestsWhenFirstRequestFailsSecondRequ
segmentLoadDropHandler.stop();
}
+ @Test(timeout = 60_000L)
+ public void testProcessBatchWithDefaultNoStorageLocations() throws Exception
+ {
+ SegmentLoadDropHandler handler = new SegmentLoadDropHandler(
+ jsonMapper,
+ new SegmentLoaderConfig(),
+ announcer,
+ Mockito.mock(DataSegmentServerAnnouncer.class),
+ segmentManager,
+ segmentCacheManager,
+ scheduledExecutorFactory.create(5, "SegmentLoadDropHandlerTest-[%d]"),
+ new ServerTypeConfig(ServerType.HISTORICAL)
+ );
+
+ handler.start();
+
+ final DataSegment segment = makeSegment("test", "1",
Intervals.of("P1d/2011-04-01"));
+
+ List<DataSegmentChangeRequest> batch = ImmutableList.of(new
SegmentChangeRequestLoad(segment));
+
+ ListenableFuture<List<DataSegmentChangeResponse>> future =
handler.processBatch(batch);
+
+ for (Runnable runnable : scheduledRunnable) {
+ runnable.run();
+ }
+
+ List<DataSegmentChangeResponse> result = future.get();
+ Assert.assertEquals(1, result.size());
+
+ SegmentChangeStatus expectedFailedStatus = SegmentChangeStatus.failed(
Review Comment:
It doesn't make sense to fail every segment load request. The historical
should just fail to start up in the first place.
One way to do that would be to add a constructor to `SegmentLoaderConfig`
and do the validation there. Another method could be to add a
`validateLocations()` method to `SegmentLoaderConfig` and invoke it from a
(probably guice-injected) class which is initialized on startup.
--
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]
