Improve error message when resource group selector cannot be loaded #13036
Conversation
wenleix
commented
Jun 30, 2019
•
wenleix
commented
wenleix
force-pushed
the
laod_rg
branch
4 times, most recently
from
26e5adf to
ee19cb3
Compare
| @@ -159,6 +169,15 @@ public void configure(ResourceGroup group, SelectionContext<VariableMap> criteri | |||
| if (lastRefresh.get() == 0) { | |||
| throw new PrestoException(CONFIGURATION_UNAVAILABLE, "Selectors cannot be fetched from database"); | |||
| } | |||
|
|
|||
| if (System.nanoTime() - lastRefresh.get() > maxRefreshInterval.toMillis() * 1_000_000) { | |||
There was a problem hiding this comment.
Put these three blocks into a helper maybe.
| refreshFailures.update(1); | ||
| log.error(e, "Error loading configuration from db"); | ||
| if (lastRefresh.get() != 0) { | ||
| log.debug("Last successful configuration loading was %s ago", succinctNanos(System.nanoTime() - lastRefresh.get()).toString()); |
There was a problem hiding this comment.
Will this line be useful in test clusters?
There was a problem hiding this comment.
@highker : "Test clusters"? -- I am thinking it will help when something goes wrong and oncall can check the log.
| @@ -119,6 +120,15 @@ public DbResourceGroupConfigurationManager(ClusterMemoryPoolManager memoryPoolMa | |||
| if (lastRefresh.get() == 0) { | |||
| throw new PrestoException(CONFIGURATION_UNAVAILABLE, "Root groups cannot be fetched from database"); | |||
| } | |||
|
|
|||
| if (System.nanoTime() - lastRefresh.get() > maxRefreshInterval.toMillis() * 1_000_000) { | |||
There was a problem hiding this comment.
The logic using succinctNanos is easier to read.
There was a problem hiding this comment.
@rongrong : But the underlying logic looks quite involved. Not sure if we want to call it per each second...
There was a problem hiding this comment.
Is it involved enough to cause coordinator perf issues? If not I won't care. If yes then change maxRefreshInterval to maxRefreshIntervalNanos instead and initialize it that way.
There was a problem hiding this comment.
Use MILLISECONDS.toNanos(1) instead of 1_000_000 to help readability :)
| // TODO: consider not throwing exception but only have a warning in this case | ||
| throw new PrestoException( | ||
| CONFIGURATION_UNAVAILABLE, | ||
| format("Selectors cannot be fetched from database. Current selectors are loaded %s ago", |
There was a problem hiding this comment.
"Root groups cannot be fetched..."
There was a problem hiding this comment.
This problem is not really fixed 😛
| @@ -119,6 +120,15 @@ public DbResourceGroupConfigurationManager(ClusterMemoryPoolManager memoryPoolMa | |||
| if (lastRefresh.get() == 0) { | |||
There was a problem hiding this comment.
I'd just change this to a checkArgument(lastRefresh.get() != 0) or remove it altogether. It's not very useful. If lastRefresh is 0 the new logic would throw anyways. You can change the error message to print something more reasonable for this special case.
There was a problem hiding this comment.
It means resource group has never successfully being fetched -- this means something is severely wrong but it could happen right?
There was a problem hiding this comment.
Well at least currently you are throwing the same exception in both cases, thus nothing distinguishes the "severity". Removing this and and modify the error message to handle lastRefresh = 0 has the same effect. If you actually believe it should be more severe, handle it accordingly. 😛
There was a problem hiding this comment.
@rongrong : Ah I see. From the code perspective is about whether "lastReferesh == 0" means something special case :).
I don't have strong opinion here, although my personal coding style will handle it separately :). Anyhow, I will put it as a special case inside "System.nanoTime() - lastRefresh.get() > maxRefreshInterval.toMillis() * 1_000_000"
...src/main/java/com/facebook/presto/resourceGroups/db/DbResourceGroupConfigurationManager.java
Outdated
Show resolved
Hide resolved
wenleix
force-pushed
the
laod_rg
branch
4 times, most recently
from
893d4bc to
6d866db
Compare
|
@rongrong : Comments addressed. Can you take another pass? ^_^ |
| if (lastRefresh.get() == 0) { | ||
| throw new PrestoException(CONFIGURATION_UNAVAILABLE, "Root groups cannot be fetched from database"); | ||
| } | ||
| checkMaxRefreshInterval(); |
There was a problem hiding this comment.
This function will print cannot fetch selectors but this should be about root groups. They are likely to fail at the same time so I'm ok with printing the same message for both, but it should not print selectors in that case.
