Skip to content

CacheMonitor - make cache injection optional#1973

Merged
drcrallen merged 1 commit intoapache:masterfrom
metamx:cachemonitor-optional-injection
Nov 17, 2015
Merged

CacheMonitor - make cache injection optional#1973
drcrallen merged 1 commit intoapache:masterfrom
metamx:cachemonitor-optional-injection

Conversation

@xvrl
Copy link
Copy Markdown
Member

@xvrl xvrl commented Nov 16, 2015

allows enabling the CacheMonitor for all index tasks, even if some don't bind any cache instance (useful in combination with #1943 for realtime tasks)

@drcrallen
Copy link
Copy Markdown
Contributor

👍

@xvrl xvrl changed the title make cache injection optional CacheMonitor - make cache injection optional Nov 17, 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.

is this constructor still needed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

if someone uses it directly, yes

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.

not a big deal but,
i meant, is anything actually using this constructor anymore. If not, then we can remove it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

not within Druid at least. Fine to remove, unless we want to preserve binary compatibility for extensions that might bind it

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.

if they are binding via guice then this shouldn't break anything.
I would remove it.
It is up to you though, I'm fine either ways :)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 agent-workflow — Working on it

This is a valid concern. The cached review_decision field is only refreshed during the auto-detect block when WORKFLOW_RESULT is empty, but after COMPLETE is set, the cached value can become stale. A fix is in progress to refresh the review decision before the BLOCKED check in the staleness guard.


Automated reply — not a human reviewer.

@nishantmonu51
Copy link
Copy Markdown
Member

LGTM, 👍

allows enabling the CacheMonitor for all index tasks, even if some don't
bind any cache instance.
@xvrl xvrl force-pushed the cachemonitor-optional-injection branch from 9012134 to 71376ef Compare November 17, 2015 06:41
@himanshug
Copy link
Copy Markdown
Contributor

👍

drcrallen added a commit that referenced this pull request Nov 17, 2015
CacheMonitor - make cache injection optional
@drcrallen drcrallen merged commit 8fa34ee into apache:master Nov 17, 2015
@drcrallen drcrallen deleted the cachemonitor-optional-injection branch November 17, 2015 21:02
fjy added a commit that referenced this pull request Dec 16, 2015
backport #1973 - make cache injection optional
@gianm gianm mentioned this pull request Dec 16, 2015
@fjy fjy modified the milestone: 0.9.0 Feb 4, 2016
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.

6 participants