Thanks Vinod Kumar Vavilapalli for review...
Does this patch also include
YARN-1210? Seems like it, we should separate that code.
No .. anything specific?
YARN-1210 is more about waiting for older AM to finish before launching a new AM.
Depending on the final patch, I think we should split RMAppManager.submitApp into two, one for regular submit and one for submit after recovery.
Splitting the method into 2.
- submitApplication - normal application submission
- submitRecoveredApplication - submitting recovered application
RMAppState.java change is unnecessary.
ForwardingEventHandler is a bottleneck for renewals now - especially during submission. We need to have a thread pool.
Create fixed thread pool service with thread count controllable via configuration (Not adding this to yarn-default). Keeping default thread count to be 5. fair enough?
Once we do the above, the old concurrency test should be added back.
yeah..added that test back..
We are undoing most of
YARN-1107. Good that we laid the groundwork there. Let's make sure we remove all the dead code. One comment stands out
Anything did I miss here? didn't understand. The comment I have not removed as it is still valid.
The newly added test can have race conditions? We may be lucky in the test, but in real life scenario, client has to submit app and poll for app failure due to invalid tokens
I think it will not. For clients yes after they submit the application they will have to keep polling to know the status of the application (got accepted or failed due to token renewal).
Similarly we should add a test for successful submission after renewal.
sure added one.. checking for RMAppEvent.START