Skip to content

Fix SQLMetadataSegmentManager to allow succesive start and stop - #5554

Merged
jihoonson merged 3 commits into
apache:masterfrom
jihoonson:fix-stop-and-start-coordinator
Mar 30, 2018
Merged

Fix SQLMetadataSegmentManager to allow succesive start and stop#5554
jihoonson merged 3 commits into
apache:masterfrom
jihoonson:fix-stop-and-start-coordinator

Conversation

@jihoonson

@jihoonson jihoonson commented Mar 29, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

SQLMetadataSegmentManager throws the below error if a coordinator loses the leadership before start() is called.

2018-03-29T00:14:55,098 INFO [Coordinator-Exec--0] io.druid.server.coordinator.DruidCoordinator - I am no longer the leader...
2018-03-29T00:14:55,098 WARN [Coordinator-Exec--0] io.druid.curator.discovery.CuratorServiceAnnouncer - Ignoring request to unannounce service[DruidNode{serviceName='druid/coordinator', host='ip-10-45-4-73.eu-central-1.compute.internal', port=-1, plaintextPort=-1, enablePlaintextPort=false, tlsPort=8281, enableTlsPort=true}]
2018-03-29T00:14:55,098 ERROR [Coordinator-Exec--0] io.druid.server.coordinator.DruidCoordinator - Caught exception, ignoring so that schedule keeps going.: {class=io.druid.server.coordinator.DruidCoordinator, exceptionType=class java.lang.IllegalMonitorStateException, exceptionMessage=Called canStop() before start()}
java.lang.IllegalMonitorStateException: Called canStop() before start()
    at io.druid.concurrent.LifecycleLock$Sync.canStop(LifecycleLock.java:166) ~[druid-common-0.12.0-iap3.jar:0.12.0-iap3]
    at io.druid.concurrent.LifecycleLock.canStop(LifecycleLock.java:256) ~[druid-common-0.12.0-iap3.jar:0.12.0-iap3]
    at io.druid.metadata.SQLMetadataSegmentManager.stop(SQLMetadataSegmentManager.java:159) ~[druid-server-0.12.0-iap3.jar:0.12.0-iap3]
    at io.druid.server.coordinator.DruidCoordinator.stopBeingLeader(DruidCoordinator.java:584) ~[druid-server-0.12.0-iap3.jar:0.12.0-iap3]
    at io.druid.server.coordinator.DruidCoordinator.access$100(DruidCoordinator.java:96) ~[druid-server-0.12.0-iap3.jar:0.12.0-iap3]
    at io.druid.server.coordinator.DruidCoordinator$CoordinatorRunnable.run(DruidCoordinator.java:631) [druid-server-0.12.0-iap3.jar:0.12.0-iap3]
    at io.druid.server.coordinator.DruidCoordinator$3.call(DruidCoordinator.java:554) [druid-server-0.12.0-iap3.jar:0.12.0-iap3]
    at io.druid.server.coordinator.DruidCoordinator$3.call(DruidCoordinator.java:547) [druid-server-0.12.0-iap3.jar:0.12.0-iap3]
    at io.druid.java.util.common.concurrent.ScheduledExecutors$2.run(ScheduledExecutors.java:102) [java-util-0.12.0-iap3.jar:0.12.0-iap3]
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) [?:1.8.0_152]
    at java.util.concurrent.FutureTask.run(FutureTask.java:266) [?:1.8.0_152]
    at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(ScheduledThreadPoolExecutor.java:180) [?:1.8.0_152]
    at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:293) [?:1.8.0_152]
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_152]
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_152]
    at java.lang.Thread.run(Thread.java:748) [?:1.8.0_152]

I added exitStopAndReset() method to LifecycleLock instead of reset() because calling two methods separately (exitStop() and reset()) isn't done atomically. Also removed unnecessary synchronizations in LookupCoordinatorManager.

This change is Reviewable

@jihoonson jihoonson added the Bug label Mar 29, 2018
jon-wei
jon-wei reviewed Mar 29, 2018
View reviewed changes
Comment thread common/src/main/java/io/druid/concurrent/LifecycleLock.java
* Resets the LifecycleLock after {@link #exitStop()}, so that {@link #canStart()} could be called again.
* Finalizes stopping the LifecycleLock and resets it, so that {@link #canStart()} could be called again. If this
* LifecycleLock is used in a restartable object, this method must be called before exit from stop() on this object,
* usually in a finally block.

@jon-wei jon-wei Mar 29, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment after this about IllegalMonitorStateException should refer to canStop() instead of exitStop()

@jihoonson jihoonson Mar 29, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Updated javadoc.

jon-wei
jon-wei reviewed Mar 29, 2018
View reviewed changes
Comment thread server/src/main/java/io/druid/server/lookup/cache/LookupCoordinatorManager.java Outdated
if (!lifecycleLock.canStart()) {
throw new ISE("LookupCoordinatorManager can't start.");
}
if (!lifecycleLock.canStart()) {

@jon-wei jon-wei Mar 29, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the comment above to show that the synchronization happens on the lock inside DruidCoordinator?

@jihoonson jihoonson Mar 29, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jon-wei the above comment makes me realized that the synchronization is really needed to prevent stop() and start() from being called at the same time. LifecycleLcok isn't enough to do that. I reverted LookupCoordinatorManager and added such a synchronization to SQLMetadataSegmentManager.

@jihoonson
jihoonson merged commit 05547e2 into apache:master Mar 30, 2018
@jon-wei jon-wei mentioned this pull request Apr 2, 2018
Closed
jon-wei pushed a commit to implydata/druid-public that referenced this pull request Apr 4, 2018
@jihoonson @jon-wei
Fix SQLMetadataSegmentManager to allow succesive start and stop (apac…
673d4b1 
…he#5554)

* Fix SQLMetadataSegmentManager to allow succesive start and stop

* address comment

* add synchronization
@gianm gianm added this to the 0.12.1 milestone Apr 9, 2018
gianm pushed a commit to gianm/druid that referenced this pull request Apr 9, 2018
@jihoonson @gianm
Fix SQLMetadataSegmentManager to allow succesive start and stop (apac…
15d83ca 
…he#5554)

* Fix SQLMetadataSegmentManager to allow succesive start and stop

* address comment

* add synchronization
@gianm gianm mentioned this pull request Apr 9, 2018
Merged
fjy pushed a commit that referenced this pull request Apr 10, 2018
@gianm @fjy
Fix SQLMetadataSegmentManager to allow succesive start and stop (#5554)…
62042c8 
… (#5605)

* Fix SQLMetadataSegmentManager to allow succesive start and stop

* address comment

* add synchronization
@jon-wei jon-wei mentioned this pull request Apr 10, 2018
Closed
@jihoonson jihoonson mentioned this pull request May 4, 2018
Closed
@leventov leventov mentioned this pull request Jul 21, 2018
Closed
@jihoonson jihoonson mentioned this pull request Jul 25, 2018
Merged
@kaka11chen

kaka11chen commented Apr 20, 2020

Copy link
Copy Markdown
Contributor

@jihoonson
Hi jihooson, why the locker of DruidCoordinator::becomeLeader() and stopBeingLeader() cannot guarantee thread-safe of multithread call? Thanks.

riovic918data pushed a commit to riovic918data/druid that referenced this pull request Jun 12, 2026
@jihoonson
Fix SQLMetadataSegmentManager to allow succesive start and stop (apac…
ff4b2e3 
…he#5554)

* Fix SQLMetadataSegmentManager to allow succesive start and stop

* address comment

* add synchronization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jon-wei jon-wei jon-wei approved these changes

Assignees

No one assigned

Labels

Bug

Projects

None yet

Milestone

0.12.1

Development

Successfully merging this pull request may close these issues.

4 participants

@jihoonson @kaka11chen @jon-wei @gianm