You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by "ZihanLi58 (via GitHub)" <gi...@apache.org> on 2023/05/05 21:21:31 UTC
[GitHub] [gobblin] ZihanLi58 opened a new pull request, #3692: [GOBBLIN-1823] Improving Container Calculation and Allocation Methodology
ZihanLi58 opened a new pull request, #3692:
URL: https://github.com/apache/gobblin/pull/3692
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
### JIRA
- [ ] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-1823
### Description
- [ ] Here are some details about my PR, including screenshots (if applicable):
**Problem**: When Yarn allocates "ghost containers" without calling the onContainerAllocated() method and when the container is eventually released, onContainersCompleted() is called, container numbers mismatches can occur.
In the onContainerAllocated() method, we add the container to the containerMap using the container ID as the key, and increase the count for the specific tag.
In the onContainersCompleted() method, we remove the container from the containerMap and decrease the count. However, in some cases, we find that the containerMap does not contain the ID, and we ignore this while still decreasing the number of the allocated tag. We do this because sometimes onContainersCompleted() is called before onContainerAllocated() for the same container.
**Solution**
1. Add the removedContainerID map to track the containers that have been released before onContainerAllocated() is called
2. Go through the container map to check the whether the assigned helix instance is alive and release it when it's in-alive for more than 10 minutes
3. Add TIME_OUT and COMPLETED as the un-retryable partition state and log it out to improve debugability.
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
Unit test for exiting function, it's hard to add a unit test for a bad yarn container and helix disconnection situation.
### Commits
- [ ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
4. Subject does not end with a period
5. Subject uses the imperative mood ("add", not "adding")
6. Body wraps at 72 characters
7. Body explains "what" and "why", not "how"
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] homatthew commented on a diff in pull request #3692: [GOBBLIN-1823] Improving Container Calculation and Allocation Methodology
Posted by "homatthew (via GitHub)" <gi...@apache.org>.
homatthew commented on code in PR #3692:
URL: https://github.com/apache/gobblin/pull/3692#discussion_r1187743610
##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java:
##########
@@ -501,12 +514,30 @@ public synchronized boolean requestTargetNumberOfContainers(YarnContainerRequest
}
}
+ //We go through all the containers we have now and check whether the assigned participant is still alive, if not, we should put them in idle container Map
Review Comment:
"Check whether assigned participant is still alive". There is nothing here to suggest that these instances are actually "assigned" anything.
I think a more accurate comment is instead something like:
> iterate through all containers allocated and check whether the corresponding helix instance is still LIVE within the helix cluster. A container that has a bad connection to zookeeper will be dropped from the Helix cluster if the disconnection is greater than the specified timeout. In these cases, we want to release the container to get a new container because these containers won't be assigned tasks by Helix
##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java:
##########
@@ -501,12 +514,30 @@ public synchronized boolean requestTargetNumberOfContainers(YarnContainerRequest
}
}
+ //We go through all the containers we have now and check whether the assigned participant is still alive, if not, we should put them in idle container Map
+ //And we will release the container if the assigned participant still offline after a given time
+
+ List<Container> containersToRelease = new ArrayList<>();
+ for (Map.Entry<ContainerId, ContainerInfo> entry : this.containerMap.entrySet()) {
+ ContainerInfo containerInfo = entry.getValue();
+ if (!HelixUtils.isInstanceLive(helixManager, containerInfo.getHelixParticipantId())) {
+ containerIdleSince.putIfAbsent(entry.getKey(), System.currentTimeMillis());
+ if (System.currentTimeMillis() - containerIdleSince.get(entry.getKey())
+ >= TimeUnit.MINUTES.toMillis(YarnAutoScalingManager.DEFAULT_MAX_CONTAINER_IDLE_TIME_BEFORE_SCALING_DOWN_MINUTES)) {
+ LOGGER.info("Releasing Container {} because the assigned participant {} has been in-active for more than {} minutes",
+ entry.getKey(), containerInfo.getHelixParticipantId(), YarnAutoScalingManager.DEFAULT_MAX_CONTAINER_IDLE_TIME_BEFORE_SCALING_DOWN_MINUTES);
+ containersToRelease.add(containerInfo.getContainer());
+ }
+ } else {
+ containerIdleSince.remove(entry.getKey());
+ }
+ }
+
// If the total desired is lower than the currently allocated amount then release free containers.
// This is based on the currently allocated amount since containers may still be in the process of being allocated
// and assigned work. Resizing based on numRequestedContainers at this point may release a container right before
// or soon after it is assigned work.
- if (numTargetContainers < totalAllocatedContainers) {
- List<Container> containersToRelease = new ArrayList<>();
+ if (containersToRelease.isEmpty() && numTargetContainers < totalAllocatedContainers) {
Review Comment:
Why do we entirely skip this block if there are already containers to release? Hopefully the previous block doesn't happen that often, but this if statement is still a bit strange to read.
To me, this should instead be:
```
if (numTargetContainers < totalAllocatedContainers - containersToRelease.size())
```
##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java:
##########
@@ -473,6 +475,17 @@ public synchronized boolean requestTargetNumberOfContainers(YarnContainerRequest
return false;
}
+ //Correct the containerMap first as there is cases that handleContainerCompletion() is called before onContainersAllocated()
+ for (ContainerId removedId :this.removedContainerID.keySet()) {
Review Comment:
nit: whitespace after `:`
##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java:
##########
@@ -473,6 +475,17 @@ public synchronized boolean requestTargetNumberOfContainers(YarnContainerRequest
return false;
}
+ //Correct the containerMap first as there is cases that handleContainerCompletion() is called before onContainersAllocated()
+ for (ContainerId removedId :this.removedContainerID.keySet()) {
+ ContainerInfo containerInfo = this.containerMap.remove(removedId);
+ if (containerInfo != null) {
+ String helixTag = containerInfo.getHelixTag();
+ allocatedContainerCountMap.putIfAbsent(helixTag, new AtomicInteger(0));
+ this.allocatedContainerCountMap.get(helixTag).decrementAndGet();
Review Comment:
put if absent 0 and then decrementing means the resulting value would be -1. That does not seem correct to me
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] ZihanLi58 commented on a diff in pull request #3692: [GOBBLIN-1823] Improving Container Calculation and Allocation Methodology
Posted by "ZihanLi58 (via GitHub)" <gi...@apache.org>.
ZihanLi58 commented on code in PR #3692:
URL: https://github.com/apache/gobblin/pull/3692#discussion_r1190482948
##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java:
##########
@@ -501,12 +514,30 @@ public synchronized boolean requestTargetNumberOfContainers(YarnContainerRequest
}
}
+ //We go through all the containers we have now and check whether the assigned participant is still alive, if not, we should put them in idle container Map
+ //And we will release the container if the assigned participant still offline after a given time
+
+ List<Container> containersToRelease = new ArrayList<>();
+ for (Map.Entry<ContainerId, ContainerInfo> entry : this.containerMap.entrySet()) {
+ ContainerInfo containerInfo = entry.getValue();
+ if (!HelixUtils.isInstanceLive(helixManager, containerInfo.getHelixParticipantId())) {
+ containerIdleSince.putIfAbsent(entry.getKey(), System.currentTimeMillis());
+ if (System.currentTimeMillis() - containerIdleSince.get(entry.getKey())
+ >= TimeUnit.MINUTES.toMillis(YarnAutoScalingManager.DEFAULT_MAX_CONTAINER_IDLE_TIME_BEFORE_SCALING_DOWN_MINUTES)) {
+ LOGGER.info("Releasing Container {} because the assigned participant {} has been in-active for more than {} minutes",
+ entry.getKey(), containerInfo.getHelixParticipantId(), YarnAutoScalingManager.DEFAULT_MAX_CONTAINER_IDLE_TIME_BEFORE_SCALING_DOWN_MINUTES);
+ containersToRelease.add(containerInfo.getContainer());
+ }
+ } else {
+ containerIdleSince.remove(entry.getKey());
+ }
+ }
+
// If the total desired is lower than the currently allocated amount then release free containers.
// This is based on the currently allocated amount since containers may still be in the process of being allocated
// and assigned work. Resizing based on numRequestedContainers at this point may release a container right before
// or soon after it is assigned work.
- if (numTargetContainers < totalAllocatedContainers) {
- List<Container> containersToRelease = new ArrayList<>();
+ if (containersToRelease.isEmpty() && numTargetContainers < totalAllocatedContainers) {
Review Comment:
Because at this point we still hold reference to those bad containers, and we might end up with releasing those containers again in this block. We can consider changing the algorism to have a hash set of containerIDtoRelease and collect the ContainerIdToRelease first and at the end of the call calculate the containerToRelease.
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] codecov-commenter commented on pull request #3692: [GOBBLIN-1823] Improving Container Calculation and Allocation Methodology
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3692:
URL: https://github.com/apache/gobblin/pull/3692#issuecomment-1536809556
## [Codecov](https://app.codecov.io/gh/apache/gobblin/pull/3692?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3692](https://app.codecov.io/gh/apache/gobblin/pull/3692?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (748e55f) into [master](https://app.codecov.io/gh/apache/gobblin/commit/7bbf6761c63055eb9ecc9f2756d4b3d68b7a1b08?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7bbf676) will **decrease** coverage by `2.25%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #3692 +/- ##
============================================
- Coverage 46.98% 44.74% -2.25%
+ Complexity 10792 2092 -8700
============================================
Files 2138 411 -1727
Lines 84065 17723 -66342
Branches 9342 2160 -7182
============================================
- Hits 39501 7930 -31571
+ Misses 40973 8935 -32038
+ Partials 3591 858 -2733
```
[see 1727 files with indirect coverage changes](https://app.codecov.io/gh/apache/gobblin/pull/3692/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] ZihanLi58 commented on a diff in pull request #3692: [GOBBLIN-1823] Improving Container Calculation and Allocation Methodology
Posted by "ZihanLi58 (via GitHub)" <gi...@apache.org>.
ZihanLi58 commented on code in PR #3692:
URL: https://github.com/apache/gobblin/pull/3692#discussion_r1190484077
##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java:
##########
@@ -473,6 +475,17 @@ public synchronized boolean requestTargetNumberOfContainers(YarnContainerRequest
return false;
}
+ //Correct the containerMap first as there is cases that handleContainerCompletion() is called before onContainersAllocated()
+ for (ContainerId removedId :this.removedContainerID.keySet()) {
+ ContainerInfo containerInfo = this.containerMap.remove(removedId);
+ if (containerInfo != null) {
+ String helixTag = containerInfo.getHelixTag();
+ allocatedContainerCountMap.putIfAbsent(helixTag, new AtomicInteger(0));
+ this.allocatedContainerCountMap.get(helixTag).decrementAndGet();
Review Comment:
I don't think we will put it though because in this case, onContainerAllocated should have already been called and we definitely have one entry in the map. The worst case is if we call onContainerAllocated and this method concurrently, then we might end up decreasing the value and then increasing it immediately.
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] homatthew commented on a diff in pull request #3692: [GOBBLIN-1823] Improving Container Calculation and Allocation Methodology
Posted by "homatthew (via GitHub)" <gi...@apache.org>.
homatthew commented on code in PR #3692:
URL: https://github.com/apache/gobblin/pull/3692#discussion_r1198417782
##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnService.java:
##########
@@ -473,6 +475,17 @@ public synchronized boolean requestTargetNumberOfContainers(YarnContainerRequest
return false;
}
+ //Correct the containerMap first as there is cases that handleContainerCompletion() is called before onContainersAllocated()
+ for (ContainerId removedId :this.removedContainerID.keySet()) {
+ ContainerInfo containerInfo = this.containerMap.remove(removedId);
+ if (containerInfo != null) {
+ String helixTag = containerInfo.getHelixTag();
+ allocatedContainerCountMap.putIfAbsent(helixTag, new AtomicInteger(0));
+ this.allocatedContainerCountMap.get(helixTag).decrementAndGet();
Review Comment:
> When Yarn allocates "ghost containers" without calling the onContainerAllocated() method and when the container is eventually released, onContainersCompleted() is called, container numbers mismatches can occur.
In the onContainerAllocated() method, we add the container to the containerMap using the container ID as the key, and increase the count for the specific tag.
This from the description makes it sound like it is not guaranteed to be called. Either way, I think if we do need to put this line, we should put if absent 1 and then decrement
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] ZihanLi58 merged pull request #3692: [GOBBLIN-1823] Improving Container Calculation and Allocation Methodology
Posted by "ZihanLi58 (via GitHub)" <gi...@apache.org>.
ZihanLi58 merged PR #3692:
URL: https://github.com/apache/gobblin/pull/3692
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org