Skip to content

Fix some problems with restoring#2141

Merged
nishantmonu51 merged 2 commits intoapache:masterfrom
gianm:fix-restoring-realtime
Dec 30, 2015
Merged

Fix some problems with restoring#2141
nishantmonu51 merged 2 commits intoapache:masterfrom
gianm:fix-restoring-realtime

Conversation

@gianm
Copy link
Copy Markdown
Contributor

@gianm gianm commented Dec 22, 2015

  • TaskLockbox should consider active tasks active even if they have no locks.
  • Shedding locks at startup is bad, we actually want to keep them. Stop doing that.
  • stopGracefully now interrupts the run thread if had started running finishJob. This avoids waiting for handoff unnecessarily.

@gianm gianm added the Bug label Dec 22, 2015
@gianm gianm added this to the 0.8.3 milestone Dec 22, 2015
@gianm gianm closed this Dec 22, 2015
@gianm gianm reopened this Dec 22, 2015
@fjy
Copy link
Copy Markdown
Contributor

fjy commented Dec 22, 2015

👍

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.

should we check if we were asked to stop before firing an alert while handling exception ?

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.

hmm, maybe only ignore it if we get an InterruptedException? other exceptions are probably worth reporting.

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.

sounds good.

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.

updated

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.

wondering if its better to not release locks only if task is restorable ?
will it be better to always release old locks if there are any in case the task is not restorable (just to be on safer side)?

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 is a reason to release locks here since,

  • we won't have any when we first start up (since a while ago, tasks don't get retried if they fail)
  • after a second startup (restore) we really do want to keep the locks as dropping them will confuse us

@gianm gianm force-pushed the fix-restoring-realtime branch from 8e6827e to 02f85fb Compare December 22, 2015 19:23
@fjy
Copy link
Copy Markdown
Contributor

fjy commented Dec 22, 2015

@gianm this has conflicts now

- Shedding locks at startup is bad, we actually want to keep them. Stop doing that.
- stopGracefully now interrupts the run thread if had started running finishJob. This avoids
  waiting for handoff unnecessarily.
@gianm gianm force-pushed the fix-restoring-realtime branch from 02f85fb to 32edd15 Compare December 23, 2015 00:05
@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Dec 23, 2015

@fjy fixed

@gianm gianm closed this Dec 23, 2015
@gianm gianm reopened this Dec 23, 2015
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.

what happened to the log line?

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.

It split in two and moved into the conditional below.

@nishantmonu51
Copy link
Copy Markdown
Member

👍

nishantmonu51 added a commit that referenced this pull request Dec 30, 2015
@nishantmonu51 nishantmonu51 merged commit df893db into apache:master Dec 30, 2015
seoeun25 added a commit to seoeun25/incubator-druid that referenced this pull request Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants