phet commented on a change in pull request #3415:
URL: https://github.com/apache/gobblin/pull/3415#discussion_r737002316
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
##########
@@ -992,9 +1005,9 @@ private void checkQuota(DagNode<JobExecutionPlan> dagNode)
throws IOException {
/**
* Increment quota by one for the given map and key.
* We need synchronization on this method because quotaMap is shared among
all the {@link DagManagerThread}s.
- * @return true if quota is not reached for this user or user is
whitelisted, false otherwise.
+ * @return an Either, which is true if quota is not reached for this user
or user is whitelisted or the job count of this user.
*/
- synchronized private boolean incrementMapAndCheckQuota(Map<String,
Integer> quotaMap, String user, DagNode<JobExecutionPlan> dagNode) {
+ synchronized private Either<Boolean, Integer>
incrementJobCountAndCheckUserQuota(Map<String, Integer> quotaMap, String user,
DagNode<JobExecutionPlan> dagNode) {
Review comment:
convention for `Either` is to have "success" on the `Right` and failure
on `Left`, so `Either<Integer, Boolean>` here. a case could be made that the
`Boolean` is redundant and you could use `Void`... but there are arguments
either way, so just a suggestion, not definitive.
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
##########
@@ -960,20 +964,29 @@ private void checkQuota(DagNode<JobExecutionPlan>
dagNode) throws IOException {
String specExecutorUri = DagManagerUtils.getSpecExecutorUri(dagNode);
boolean proxyUserCheck = true;
if (proxyUser != null) {
- proxyUserCheck = incrementMapAndCheckQuota(proxyUserToJobCount,
proxyUser, dagNode);
+ Either<Boolean, Integer> proxyQuotaTest =
incrementJobCountAndCheckUserQuota(proxyUserToJobCount, proxyUser, dagNode);
+ proxyUserCheck = proxyQuotaTest.get() instanceof Either.Left;
Review comment:
I didn't realize gobblin has our own `Either` implementation. to my
eyes, it's incomplete, since it promotes untyped access (by `Object get()`
here). missing are a pair of predicate accessors: `boolean isLeft()` and
`boolean isRight()`.
used here though, `proxyUserCheck` will always be false, since exactly one
of these could be true:
```
proxyQuotaTest.get() instanceof Boolean
proxyQuotaTest.get() instanceof Integer
```
to step back: sorry, I know I suggested `Either`... but I didn't realize
guava doesn't provide one: must supplement w/ fugue:
https://bitbucket.org/atlassian/fugue/src/master/fugue/src/main/java/io/atlassian/fugue/Either.java
next steps:
1. either augment gobblin's `Either` (and use `Either<Integer, Void>`)
2. use `Optional<Integer>` instead, where `None` is interpreted as success
(i.e. there is no job count already at max quota)
I'd lean toward the second option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
