Alias #267
Alias #267
Conversation
| public static final String ALIAS_PATH = "/query/alias"; | ||
| private final SpawnDataStore spawnDataStore; | ||
| private final HashMap<String, List<String>> alias2jobs; | ||
| private final HashMap<String, String> job2alias; |
There was a problem hiding this comment.
Why not use a BiMap? That's cleaner IMO.
| public AliasManagerImpl(SpawnDataStore spawnDataStore) throws Exception { | ||
| this.spawnDataStore = spawnDataStore; | ||
| this.mapLock = new ReentrantLock(); | ||
| this.mapper = new ObjectMapper(); |
| alias2jobs.put(alias, jobs); | ||
| job2alias.put(jobs.get(0), alias); | ||
| StringWriter sw = new StringWriter(); | ||
| mapper.writeValue(sw, jobs); |
There was a problem hiding this comment.
If you use writeValueAsString(jobs) you wont need a StringWriter
| try { | ||
| updateAlias(alias, this.spawnDataStore.getChild(ALIAS_PATH, alias)); | ||
| } catch (ExecutionException e) { | ||
| log.warn("Failed to refresh alias: {}", alias, e); |
| } catch (ExecutionException e) { | ||
| log.warn("Failed to refresh alias: {}", alias, e); | ||
| } catch (Exception e) { | ||
| log.error("",e); |
There was a problem hiding this comment.
"" isn't super useful for debugging. Something like Unexpected error refreshing alias <alias> would be better.
| } | ||
| List<String> jobs = decodeAliases(data); | ||
| if (jobs.isEmpty()) { | ||
| log.warn("no jobs for alias {}, ignoring {}", alias, alias); |
There was a problem hiding this comment.
ignoring alias {} since there are no jobs or something would avoid the redundant alias log entry.
| @VisibleForTesting | ||
| protected List<String> decodeAliases(@Nonnull Object data) { | ||
| try { | ||
| return mapper.readValue(data.toString(), new TypeReference<List<String>>() {}); |
There was a problem hiding this comment.
Is data always a string here? If so why is it an object paramater?
| * @param job The job to test | ||
| * @param alias The alias to check | ||
| */ | ||
| private void checkAlias(String job, String alias) { |
There was a problem hiding this comment.
This method also has unclear side effects since it removes job from job2alias when the method name is called checkAlias
| * @param jobid The jobId to check | ||
| * @return One of the aliases for that job | ||
| */ | ||
| public String getLikelyAlias(String jobid) { |
There was a problem hiding this comment.
why likely alias? Wouldn't this be the alias?
| } | ||
| } finally { | ||
| mapLock.unlock(); | ||
| } |
There was a problem hiding this comment.
Shouldn't you copy the maps first? What happens if the deleteChild call below fails? You now have a bad state.
| public void deleteAlias(String alias) { | ||
| mapCache.remove(alias); | ||
| } | ||
| } |
There was a problem hiding this comment.
FYI, There is weird symbol right next to right curly bract in line 104. Cannot remove it from PR, but it's ok in source code.
There was a problem hiding this comment.
If you mouse over the 'weird symbol' it will tell you what it means. Please try this.
No description provided.