Don't update segment metadata if archive doesn't move anything #3476

drcrallen · 2016-09-21T04:24:49Z

Expands the behavior of the archiver to handle when things don't actually move. In such a scenario metadata segment update actions are not attempted.

If an Archiver implementation does not honor the new interface contract, then the prior behavior is still preserved.

drcrallen · 2016-09-21T15:35:50Z

I totally didn't mean to push this to a druid-io branch, sorry about that.

drcrallen · 2016-09-21T15:36:41Z

Odd failure:

Tests run: 12, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.605 sec <<< FAILURE! - in io.druid.server.lookup.PollingLookupTest
testApplyAfterDataChange[1](io.druid.server.lookup.PollingLookupTest)  Time elapsed: 0.133 sec  <<< FAILURE!
java.lang.AssertionError: non-null check expected:<bar> but was:<null>
    at org.junit.Assert.fail(Assert.java:88)
    at org.junit.Assert.failNotEquals(Assert.java:743)
    at org.junit.Assert.assertEquals(Assert.java:118)
    at io.druid.server.lookup.PollingLookupTest.assertMapLookup(PollingLookupTest.java:205)
    at io.druid.server.lookup.PollingLookupTest.testApplyAfterDataChange(PollingLookupTest.java:148)

https://travis-ci.org/druid-io/druid/jobs/161527673

drcrallen · 2016-09-21T17:45:21Z

Tests run: 5, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 67.912 sec <<< FAILURE! - in io.druid.curator.announcement.AnnouncerTest
testSanity(io.druid.curator.announcement.AnnouncerTest)  Time elapsed: 61.081 sec  <<< ERROR!
java.lang.Exception: test timed out after 60000 milliseconds
    at sun.misc.Unsafe.park(Native Method)
    at java.util.concurrent.locks.LockSupport.parkNanos(LockSupport.java:215)
    at java.util.concurrent.locks.AbstractQueuedSynchronizer.doAcquireSharedNanos(AbstractQueuedSynchronizer.java:1037)
    at java.util.concurrent.locks.AbstractQueuedSynchronizer.tryAcquireSharedNanos(AbstractQueuedSynchronizer.java:1328)
    at java.util.concurrent.CountDownLatch.await(CountDownLatch.java:277)
    at org.apache.curator.test.Timing.awaitLatch(Timing.java:120)
    at io.druid.curator.announcement.AnnouncerTest.testSanity(AnnouncerTest.java:101)

https://travis-ci.org/druid-io/druid/jobs/161663853

different failure

drcrallen · 2016-09-21T19:03:01Z

Tests run: 12, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.685 sec <<< FAILURE! - in io.druid.server.lookup.PollingLookupTest
testApplyAfterDataChange[1](io.druid.server.lookup.PollingLookupTest)  Time elapsed: 0.223 sec  <<< FAILURE!
java.lang.AssertionError: non-null check expected:<bar> but was:<null>
    at org.junit.Assert.fail(Assert.java:88)
    at org.junit.Assert.failNotEquals(Assert.java:743)
    at org.junit.Assert.assertEquals(Assert.java:118)
    at io.druid.server.lookup.PollingLookupTest.assertMapLookup(PollingLookupTest.java:205)
    at io.druid.server.lookup.PollingLookupTest.testApplyAfterDataChange(PollingLookupTest.java:148)

failed again https://travis-ci.org/druid-io/druid/jobs/161700091

leventov · 2016-10-13T13:00:13Z

api/src/main/java/io/druid/segment/loading/DataSegmentArchiver.java

+   *
+   * @throws SegmentLoadingException on error
+   */
+  DataSegment archive(DataSegment segment) throws SegmentLoadingException;


Maybe annotate with @javax.annotation.Nullable? This will make IDE to warn you about using the object, returned from this method, without null check.

leventov · 2016-10-13T13:00:40Z

api/src/main/java/io/druid/segment/loading/DataSegmentArchiver.java

+   *
+   * @throws SegmentLoadingException on error
+   */
+  DataSegment restore(DataSegment segment) throws SegmentLoadingException;


Maybe annotate with @javax.annotation.Nullable?

leventov · 2016-10-13T13:21:46Z

indexing-service/src/main/java/io/druid/indexing/common/task/RestoreTask.java


-      // Move segments
-      for (DataSegment segment : unusedSegments) {
+    List<DataSegment> restoredSegments = Lists.newLinkedList();


I think LinkedList is pointless here. It's not a performance-critical place; but there should always be a reason to use LinkedList rather than default ArrayList. When reader sees LinkedList, he spends mental cycles trying to understand, why it is used here. It's confusing when there is no apparent reason.

Agreed, I don't remember why that was here.

leventov · 2016-10-13T13:22:32Z

indexing-service/src/main/java/io/druid/indexing/common/task/RestoreTask.java

+      final DataSegment restored = toolbox.getDataSegmentArchiver().restore(segment);
+      if (restored == null) {
+        log.info("Segment [%s] did not move, not updating metadata", segment);
+      } else {
        restoredSegments.add(toolbox.getDataSegmentArchiver().restore(segment));


Maybe should use the restored variable here rather than computing toolbox.getDataSegmentArchiver().restore(segment) again?

yes we should, fixing

leventov · 2016-10-13T13:25:44Z

indexing-service/src/main/java/io/druid/indexing/common/task/RestoreTask.java

+    // Move segments
+    for (DataSegment segment : unusedSegments) {
+      final DataSegment restored = toolbox.getDataSegmentArchiver().restore(segment);
+      if (restored == null) {


For consistency with ArchiveTask, maybe restored != null branch should go first?

leventov · 2016-10-27T23:41:32Z

indexing-service/src/main/java/io/druid/indexing/common/task/RestoreTask.java

@@ -22,18 +22,17 @@
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Lists;
-
+import com.metamx.common.ISE;


Use new package, since java-util has moved?

leventov · 2016-11-02T17:47:54Z

👍 after Travis

fjy · 2016-11-11T18:41:43Z

👍

…e#3476)

gianm · 2016-11-16T19:24:37Z

api/src/main/java/io/druid/segment/loading/DataSegmentArchiver.java

+   * @throws SegmentLoadingException on error
+   */
+  @Nullable
+  DataSegment archive(DataSegment segment) throws SegmentLoadingException;


Instead of returning null, perhaps a nicer API would be to return an equivalent object. Then callers don't need to deal with potentially null returns. If a caller wants to know if anything was actually done, they could check retVal.equals(segment) rather than retVal != null.

That's pretty challenging since loadSpec is just a Map<String, Object> which may have whatever junk in it. I opted for this way of doing it so that the implementation can make the determination on if the segment was moved or not.

Also see #3287 and #3286 for semi-related

Oh yeah, I forgot DataSegment identity is based around the segment identifier. OK then in that case what I said is nonsense.

…e#3476)

gianm

👍

drcrallen added 3 commits September 20, 2016 21:20

Don't update segment metadata if archive doesn't move anything

62d918d

Fix restore task to handle potential null values

3ba410f

Don't try to update empty metadata

5e52f1f

drcrallen added the Improvement label Sep 21, 2016

drcrallen added this to the 0.9.3 milestone Sep 21, 2016

drcrallen closed this Sep 21, 2016

drcrallen reopened this Sep 21, 2016

drcrallen closed this Sep 21, 2016

drcrallen reopened this Sep 21, 2016

drcrallen closed this Sep 21, 2016

drcrallen reopened this Sep 21, 2016

drcrallen mentioned this pull request Sep 21, 2016

Test failure - io.druid.server.lookup.PollingLookupTest.testApplyAfterDataChange #3480

Closed

leventov requested changes Oct 13, 2016

View reviewed changes

drcrallen added 2 commits October 21, 2016 16:06

Address review comments

1858bbf

Merge remote-tracking branch 'druid/master' into smarterArchiver

e54c0ac

leventov requested changes Oct 27, 2016

View reviewed changes

drcrallen added 2 commits November 2, 2016 09:55

Merge remote-tracking branch 'origin/master' into smarterArchiver

452f732

Move to druid-io java-util

5a9001f

leventov added a commit to metamx/druid that referenced this pull request Nov 11, 2016

Don't update segment metadata if archive doesn't move anything (apach…

8ed7660

…e#3476)

gianm reviewed Nov 16, 2016

View reviewed changes

leventov added a commit to metamx/druid that referenced this pull request Nov 22, 2016

Don't update segment metadata if archive doesn't move anything (apach…

8b1cde4

…e#3476)

gianm assigned fjy and gianm Nov 22, 2016

gianm approved these changes Dec 1, 2016

View reviewed changes

gianm merged commit 27ab23e into master Dec 1, 2016

gianm deleted the smarterArchiver branch December 1, 2016 15:49

clambertus unassigned fjy and gianm Jul 6, 2018

seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Feb 25, 2022

apache#3476 Support parquet schema resolver

dc57d5a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't update segment metadata if archive doesn't move anything #3476

Don't update segment metadata if archive doesn't move anything #3476

drcrallen commented Sep 21, 2016 •

edited

Loading

drcrallen commented Sep 21, 2016

drcrallen commented Sep 21, 2016

drcrallen commented Sep 21, 2016

drcrallen commented Sep 21, 2016

leventov Oct 13, 2016

leventov Oct 13, 2016

leventov Oct 13, 2016

drcrallen Oct 21, 2016

leventov Oct 13, 2016

drcrallen Oct 21, 2016

leventov Oct 13, 2016

drcrallen Oct 21, 2016

leventov Oct 27, 2016

drcrallen Nov 2, 2016

leventov commented Nov 2, 2016

fjy commented Nov 11, 2016

gianm Nov 16, 2016

drcrallen Nov 28, 2016

drcrallen Nov 28, 2016

gianm Dec 1, 2016

gianm left a comment

Don't update segment metadata if archive doesn't move anything #3476

Don't update segment metadata if archive doesn't move anything #3476

Conversation

drcrallen commented Sep 21, 2016 • edited Loading

drcrallen commented Sep 21, 2016

drcrallen commented Sep 21, 2016

drcrallen commented Sep 21, 2016

drcrallen commented Sep 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leventov commented Nov 2, 2016

fjy commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianm left a comment

Choose a reason for hiding this comment

drcrallen commented Sep 21, 2016 •

edited

Loading