Default value is 100 as defined in YarnConfiguration

Submitting patch to Jenkins for pre-commit build.

-1 overall

This message was automatically generated.

hi Haibo Chen,
Thanks for working on this patch and yes it would be better to limit the number of threads in the executor service. But few nits/queries in the patch :

• if YarnConfiguration is modified then better yarn-default.xml is also modified, but in the first place do we require to keep it configurable ? IMO just having a fixed value like 50 should be safe.
• threadPool can be of default access instead of public , so that only accessible to testcases
• Dint understand the need of Semaphore, as in the Runnable immediately after semaphore.acquire() we release in the finally block. And even if not i thought we can submit multiple runnables (say 5/10) with a short sleep and check whether number of live threads having thread name as LogAggregationService is only 1 right ?

Hi Naganarasimha G R,

Thanks very much for your comments. I have addressed the threadPool accessibility issue and also modified yarn-default.xml to match YarnConfiguration. To answer your other comments:

1. Yes, 50 should be safe. (The default I set is 100). But maybe sometimes even 50 threads alone for log aggregation is too much resource dedicated? Some users may also want to use more than 50 if they have powerful machines and many yarn applications? If this is configurable, users themselves can decide.

2. The purpose of the semaphore is to block the threads in the thread pool because the main thread always acquire the semaphore first. Because I set the thread pool size to be 1, once that single thread tries to acquire the semaphore when it executes either of the two runnable, it blocks and the other runnable will not be executed if the thread pool can indeed create only 1 thread. (If another thread is available in the thread pool, there will be another thread blocking on the semaphore, failing the test). The immediate release after acquire in runnable is just to safely release the resource. I'll try to add comments in the test code.

+1 overall

This message was automatically generated.

Haibo Chen should we do some range checking on the setting for the pool size? Out of range values can cause runtime exceptions and that could leave us with a NM that can not start.

• Setting the thread number config to 0 the creation of the pool will fail
• Anything that is NaN or too large the conf.getInt() will throw

Invalid values should cause us to use the default value.

Besides Wilfred Spiegelenburg's comments, I have some feedback on the unit test:

• We should use more than 1 thread in the thread pool because 1 of something can sometimes hide problems. Something like 3 would be better.
• In case something goes wrong, it would be good to:
  • add a timeout to the test @Test(timeout=30000)
  • make the threads not block indefinitely. That can be done by using tryAcquire(long timeout, TimeUnit unit) instead of just acquire(). If you make the timeout for the threads longer than the timeout for the test itself, you won't have to worry about any timing problems with the thread exiting early, while still preventing the threads from possibly hanging forever
• The way you're searching for threads is okay, but it would be better if we could get them directly from the thread pool. I see that LogAggregationService only exposes an ExecutorService for the thread pool, but looking at how it's made, I believe it's actually a ThreadPoolExecutor underneath. Can you try casting to ThreadPoolExecutor and see if that works? ThreadPoolExecutor has methods to check how many threads are running etc. If that doesn't work, then I'm okay with the current approach.

Thanks very much for you guys' comments. I have updated the patch accordingly.

Looks good overall. A few minor things:

• Add a unit test for NaN and negative values for NM_LOG_AGGREGATION_THREAD_POOL_SIZE to verify it uses the default value.
• I thought about the unit test some more, and because the Semaphore has a count of 1, if the test behaves properly long before the timeout occurs, each thread will still have to sit through the others' acquiring the semaphore (or the timeout). It would be faster if we use a ReadWriteLock instead, with the test acquiring write lock and the threads acquiring read locks. This way, the threads won't block each other.
• There should be a try-finally with the acquire/release in the test.

-1 overall

This message was automatically generated.

New unit tests added for invalid values. Other comments addressed as well

+1 overall

This message was automatically generated.

LGTM. +1 pending Jenkins (which I just kicked off because it didn't seem to notice for some reason...)
Will commit tomorrow unless anyone has any other comments.

I guess I just missed that build and hadn't refreshed. I've canceled the extra one I just kicked off

+1 overall

This message was automatically generated.

+1 LGTM

Thanks Haibo and everyone for reviews. Committed to trunk and branch-2!

FAILURE: Integrated in Hadoop-trunk-Commit #9364 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9364/)
YARN-4697. NM aggregation thread pool is not bound by limits (haibochen (rkanter: rev 954dd57043d2de4f962876c1b89753bfc7e4ce55)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/logaggregation/TestLogAggregationService.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java
• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/resources/yarn-default.xml
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/logaggregation/LogAggregationService.java

Thanks Haibo Chen fixed this problem.

I think this is a critical issue that could cause issues when NM restart with recovery enabled.

I think we should backport this fix to 2.6+ releases.

Thoughts?

In the case that we have had a problem with log aggregation this could cause a problem on restart. The number of threads created at that point could be huge and will put a large load on the NameNode and in worse case could even bring it down due to file descriptor issues.

Haibo Chen / Robert Kanter / Varun Vasudev / Wangda Tan, while I agree with a general notion that the thread pool limits are good, I actually fail to see how this problem is happening. The number of log-aggregation threads should be limited by the number of concurrent applications running and finishing in a cluster which should be in the order of thousands. Is there something special happening at restart time?

My concern is that if don't fix the root-cause, though we've protected ourselves from crashes, we'd just be queueing a lot of aggregation processes and causing long waiting times.

The other things we should do with this patch is that each thread should identify the current application being aggregated, so that we can debug issues better.

IIUC, currently all application recovery go to INIT state, and AppInitTransition sends a event to LogAggregationService and a new thread is required.

Not entirely sure about what's the best fix for this issue, can someone advice?

Vinod Kumar Vavilapalli Upon NM restart, NM will try to recover all applications and submit a log aggregation task to the thread pool for each application recovered. Therefore, a large number of recovered applications plus concurrent applications can cause the thread pool to increase without a bound.

My concern is that if don't fix the root-cause, though we've protected ourselves from crashes, we'd just be queueing a lot of aggregation processes and causing long waiting times.

Agree. We do see NM log aggregation service launch many active threads which keep large number of TCP connections to DN which use out system's file limit. We can fix shared limited thread number here, but the TCP connections problem may not solved by this patch.

Upon NM restart, NM will try to recover all applications and submit a log aggregation task to the thread pool for each application recovered. Therefore, a large number of recovered applications plus concurrent applications can cause the thread pool to increase without a bound.

Does all these applications are active one or finished already? I suspect we are leaking finished applications in NM state store in recover process. I noticed this issue in filing YARN-4325 but lost my progress as previous long running cluster is gone. Haibo Chen, could you check if your case is the same here?

In general, I think the fix on this JIRA is OK. But I agree with Vinod that we should dig out more on the root cause or it could be other holes (like TCP connection leaking mentioned above).

After investigating on our cluster which hit the same issue also recently, I think two root cause here:
1. Due to YARN-4325, too many stale applications doesn't get purged from NM state store clean. So NM recover too many stale application which will init app with log aggregation.
2. Because these applications are stale, some operation - like: createAppDir() will failed with token issues, but we swallow exception there and continue to create invalid aggregator - just file YARN-4984 to fix this issue.