[
https://issues.apache.org/jira/browse/YARN-10532?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17289968#comment-17289968
]
Peter Bacsko commented on YARN-10532:
-------------------------------------
FIRST round review.
I might post more but these are that stand out to me right now.
1.
AbstractYarnScheduler:
{noformat}
public void removeQueue(CSQueue queueName) throws YarnException {
throw new YarnException(getClass().getSimpleName()
+ " does not support removing queues");
}
{noformat}
If this is an abstract class, just make this method abstract without
implementation:
{{public abstract void removeQueue(CSQueue queueName) throws YarnException;}}
2.
{noformat}
// When this queue has application submit to?
// This property only applies to dynamic queue,
// and will be used to check when the queue need to be removed.
{noformat}
Rephrase this comment a little bit:
{noformat}
// The timestamp of the last submitted application to this queue.
// Only applies to dynamic queues.
{noformat}
3.
{noformat}
// "Tab" the queue, so this queue won't be removed because of idle
timeout.
public void signalToSubmitToQueue() {
{noformat}
I'd comment that "Update the timestamp of the last submitted application".
Also, the method name is sounds weird to me. What it does is really simple.
Call it {{updateLastSubmittedTimeStamp()}}.
If you use the right naming, then the comment is probably unnecessary. We
don't need comments if the method is simple and easy to understand its purpose.
4. Instead of this:
{noformat}
// just for test
public void setLastSubmittedTimestamp(long lastSubmittedTimestamp) {
{noformat}
use this:
{noformat}
@VisibleForTesting
public void setLastSubmittedTimestamp(long lastSubmittedTimestamp) {
{noformat}
5. This comment is completely unnecessary I think:
{noformat}
// Expired queue, when there are no applications in queue,
// and the last submit time has been expired.
// Delete queue when expired deletion enabled.
{noformat}
It's obvious what the method is doing. Or if you insist on having a comment
there, just add "Timeout expired, delete the dynamic queue"
6. I suggest a better exception message:
{noformat}
throw new SchedulerDynamicEditException(
"The queue " + queue.getQueuePath()
+ " can't removed normally.");
{noformat}
It should say "The queue ABC cannot be removed because it's parent is null".
7. {{LOG.info("Removed queue: " + queue.getQueuePath());}} – not necessary to
log a successful removal. If there is no message, it means that the removal was
successful.
8. Typo in comment: {{// 300s for expired defualt}} --> "default"
9. These methods are used by the code itself, not just test:
{noformat}
@VisibleForTesting
public void prepareForAutoDeletion() {
...
@VisibleForTesting
public void triggerAutoDeletionForExpiredQueues() {
{noformat}
So "VisibleForTesting" should be removed.
10.
{noformat}
private void queueAutoDeletion(CSQueue checkQueue) {
//Scheduler update is asynchronous
if (checkQueue != null) {
{noformat}
Three things:
* {{queueAutoDeletion()}} - this method is a noun. Ideally, methods begin with
a verb. For example "deleteDynamicQueue()" or "deleteAutoCreatedQueue()".
* Also, why is it called "checkQueue"? Just call it "queue".
* The comment is confusing: "Scheduler update is asynchronous". Why is it
there? This statement does not tell me anything in this context. Does it refer
to the null-check?
11.
{noformat}
@Before
public void setUp() throws Exception {
// The expired time for deletion will be 1s
super.setUp();
}
{noformat}
This method is unnecessary, the setUp() method in the super class will be
called anyway.
12. Test methods: {{testEditSchedule}},
{{testCapacitySchedulerAutoQueueDeletion}},
{{testCapacitySchedulerAutoQueueDeletionDisabled}}
These test methods are long, but it's not my main problem. There are
{{Thread.sleep()}} calls inside. This is really problematic, especially short
sleeps like {{Thread.sleep(100)}}.
I have fixed many flaky tests where the test code were full of
{{Thread.sleep()}}. This must be avoided whever possible.
We should come up with a better solution, eg. polling a certain state
regularly, for example:
{noformat}
GenericTestUtils.waitFor(someObject.isConditionTrue(), 500, 10_000);
{noformat}
This method calls {{someObject.isConditionTrue()}} in every 500ms and it times
out after 10 seconds. In case of a timeout, a {{TimeoutException}} will be
thrown.
> Capacity Scheduler Auto Queue Creation: Allow auto delete queue when queue is
> not being used
> --------------------------------------------------------------------------------------------
>
> Key: YARN-10532
> URL: https://issues.apache.org/jira/browse/YARN-10532
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Wangda Tan
> Assignee: Qi Zhu
> Priority: Major
> Attachments: YARN-10532.001.patch, YARN-10532.002.patch,
> YARN-10532.003.patch, YARN-10532.004.patch, YARN-10532.005.patch,
> YARN-10532.006.patch, YARN-10532.007.patch, YARN-10532.008.patch,
> YARN-10532.009.patch, YARN-10532.010.patch, YARN-10532.011.patch,
> YARN-10532.012.patch, YARN-10532.013.patch, YARN-10532.014.patch,
> YARN-10532.015.patch, YARN-10532.016.patch, YARN-10532.017.patch,
> YARN-10532.018.patch, YARN-10532.019.patch, YARN-10532.020.patch,
> YARN-10532.021.patch, image-2021-02-12-21-32-02-267.png
>
>
> It's better if we can delete auto-created queues when they are not in use for
> a period of time (like 5 mins). It will be helpful when we have a large
> number of auto-created queues (e.g. from 500 users), but only a small subset
> of queues are actively used.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
