Skip to content

Properly shutdown SQLMetadataSegmentManager and SQLMetadataRuleManager#1366

Merged
xvrl merged 1 commit intoapache:masterfrom
metamx:futurizeSQLMetadataSegmentManager
May 16, 2015
Merged

Properly shutdown SQLMetadataSegmentManager and SQLMetadataRuleManager#1366
xvrl merged 1 commit intoapache:masterfrom
metamx:futurizeSQLMetadataSegmentManager

Conversation

@drcrallen
Copy link
Copy Markdown
Contributor

In some unit tests I was doing rapid starting and stopping of the SQLMetadataSegmentManager

It was logging errors which did not need to be logged because the tasks were trying to resubmit themselves after the shutdown and failing (as they should). This is not technically an error though, and this PR uses futures to allow killing of the task before shutdown to help minimize false-errors in logs.

@xvrl xvrl changed the title Add futures to SQLMetadataSegmentManager Properly shutdown SQLMetadataSegmentManager May 14, 2015
@drcrallen drcrallen closed this May 15, 2015
@drcrallen drcrallen reopened this May 15, 2015
@xvrl
Copy link
Copy Markdown
Member

xvrl commented May 15, 2015

@drcrallen should we update the rulemanager with the same fix as well?

@drcrallen drcrallen force-pushed the futurizeSQLMetadataSegmentManager branch from b67abb2 to 74be31a Compare May 15, 2015 22:38
@drcrallen drcrallen changed the title Properly shutdown SQLMetadataSegmentManager Properly shutdown SQLMetadataSegmentManager and SQLMetadataRuleManager May 15, 2015
@drcrallen drcrallen force-pushed the futurizeSQLMetadataSegmentManager branch from 74be31a to e6809ac Compare May 15, 2015 23:08
@drcrallen drcrallen force-pushed the futurizeSQLMetadataSegmentManager branch from e6809ac to 051c3cc Compare May 15, 2015 23:08
xvrl added a commit that referenced this pull request May 16, 2015
Properly shutdown SQLMetadataSegmentManager and SQLMetadataRuleManager
@xvrl xvrl merged commit 21ba859 into apache:master May 16, 2015
@xvrl xvrl deleted the futurizeSQLMetadataSegmentManager branch May 16, 2015 00:33
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.

Should this emit an alert?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe this is just replicating the existing behavior, which was handled within ScheduledExecutors.scheduleWithFixedDelay

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nothing against having an alert though

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.

I don't think there should be an alert here without a retry strategy. An acute network hiccup should not cause an alert if it is simply recoverable.

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.

I filed #1380 to track the request

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I just realized this error will never get logged, SQLMetadataSegmentManager.poll() already logs and swallows all exceptions

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants