[FLINK-37689] Improve the comments and variable names for TaskMailbox #26469
Conversation
|
|
||
| /** | ||
| * The maximal priority for mails. This priority indicates that the message should be performed | ||
| * The maximal priority for mails. This priority indicates that the mail should be performed |
There was a problem hiding this comment.
message -> mail. mail is a more suitable name in the context of TaskMailbox.
| * Returns an optional with either the oldest mail from the mailbox if the mailbox is not empty | ||
| * or an empty optional otherwise. Note that the priority is given to retrieving email from the | ||
| * head of batch, and when email cannot be retrieved from the batch, it is retrieved from the | ||
| * head of queue. This also means that emails in batch are older than emails in queue. |
There was a problem hiding this comment.
Adds comments to increasing the readability of tryTake and take. These comments show below.
Note that the priority is given to retrieving email from the head of batch, and when email cannot be retrieved from the batch, it is retrieved from the head of queue. This also means that emails in batch are older than emails in queue.
| * @return an optional with either the oldest mail from the mailbox (head of queue) if the | ||
| * mailbox is not empty or an empty optional otherwise. | ||
| * @throws IllegalStateException if mailbox is already closed. | ||
| * @throws MailboxClosedException if mailbox is already closed. |
There was a problem hiding this comment.
Supplement the missing exception into comments.
| * even if no batch had been created. | ||
| * | ||
| * <p>The default batch is empty. Thus, this method must be invoked once before {@link | ||
| * <p>By default, the batch is empty. Thus, this method must be invoked once before {@link |
There was a problem hiding this comment.
The default batch is empty. -> By default, the batch is empty.. The latter is more accurate.
|
|
||
| /** | ||
| * Returns an optional with either the oldest mail from the batch (head of queue) if the batch | ||
| * Returns an optional with either the oldest mail from the batch (head of batch) if the batch |
There was a problem hiding this comment.
(head of queue) -> (head of batch). We only fetch the head mail from the batch not the queue with tryTakeFromBatch.
| lock.unlock(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Move the size() in TaskMailboxImpl so that it follows the order of the declaration in the interface TaskMailbox.
| return Optional.of(head); | ||
| Mail headEmail = takeOrNull(batch, priority); | ||
| if (headEmail != null) { | ||
| return Optional.of(headEmail); |
There was a problem hiding this comment.
head -> headEmail. The latter is a better variable name. Some code uses headEmail and other uses head before this PR. We should unify the name.
github-actions
bot
added
community-reviewed
github-actions
bot
added
community-reviewed
2 participants
What is the purpose of the change
This PR aims to improve the comments and variable names for
TaskMailbox.There are some improvement points show below.
message->mail.mailis a more suitable name in the context ofTaskMailbox.Adds comments to increasing the readability of
tryTakeandtake. These comments show below.Note that the priority is given to retrieving email from the head of batch, and when email cannot be retrieved from the batch, it is retrieved from the head of queue. This also means that emails in batch are older than emails in queue.Supplement the missing exception into comments.
The default batch is empty.->By default, the batch is empty.. The latter is more accurate.(head of queue)->(head of batch). We only fetch the head mail from thebatchnot thequeuewithtryTakeFromBatch.head->headEmail. The latter is a better variable name. Some code usesheadEmailand other usesheadbefore this PR. We should unify the name.Move the size() in
TaskMailboxImplso that it follows the order of the declaration in the interfaceTaskMailbox.Brief change log
Improve the comments and variable names for
TaskMailbox.Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation