You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "beliefer (via GitHub)" <gi...@apache.org> on 2025/04/16 09:53:25 UTC
[PR] [FLINK-37689] Improve the comments and variable names for TaskMailbox [flink]
beliefer opened a new pull request, #26469:
URL: https://github.com/apache/flink/pull/26469
## 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`. `mail` is a more suitable name in the context of `TaskMailbox`.
- 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.`
- 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 the `batch` not the `queue` with `tryTakeFromBatch`.
- `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.
- Move the size() in `TaskMailboxImpl` so that it follows the order of the declaration in the interface `TaskMailbox`.
## 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:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no)
- The serializers: (no)
- The runtime per-record code paths (performance sensitive): (no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
- The S3 file system connector: (no)
## Documentation
- Does this pull request introduce a new feature? (no)
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37689] Improve the comments and variable names for TaskMailbox [flink]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #26469:
URL: https://github.com/apache/flink/pull/26469#discussion_r2046564313
##########
flink-runtime/src/main/java/org/apache/flink/streaming/runtime/tasks/mailbox/TaskMailboxImpl.java:
##########
@@ -96,37 +96,26 @@ public boolean hasMail() {
return !batch.isEmpty() || hasNewMail;
}
- @Override
- public int size() {
- final ReentrantLock lock = this.lock;
- lock.lock();
- try {
- return batch.size() + queue.size();
- } finally {
- lock.unlock();
- }
- }
-
@Override
public Optional<Mail> tryTake(int priority) {
checkIsMailboxThread();
checkTakeStateConditions();
- Mail head = takeOrNull(batch, priority);
- if (head != null) {
- return Optional.of(head);
+ Mail headEmail = takeOrNull(batch, priority);
+ if (headEmail != null) {
+ return Optional.of(headEmail);
Review 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.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37689] Improve the comments and variable names for TaskMailbox [flink]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #26469:
URL: https://github.com/apache/flink/pull/26469#discussion_r2046562646
##########
flink-runtime/src/main/java/org/apache/flink/streaming/runtime/tasks/mailbox/TaskMailbox.java:
##########
@@ -128,7 +134,7 @@ public interface TaskMailbox {
* not affect {@link #tryTake(int)} and {@link #take(int)}; that is, they return the same mails
* 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
Review Comment:
`The default batch is empty.` -> `By default, the batch is empty.`. The latter is more accurate.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37689] Improve the comments and variable names for TaskMailbox [flink]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #26469:
URL: https://github.com/apache/flink/pull/26469#discussion_r2046561417
##########
flink-runtime/src/main/java/org/apache/flink/streaming/runtime/tasks/mailbox/TaskMailbox.java:
##########
@@ -97,26 +97,32 @@ public interface TaskMailbox {
boolean hasMail();
/**
- * Returns an optional with either the oldest mail from the mailbox (head of queue) if the
- * mailbox is not empty or an empty optional otherwise.
+ * 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.
Review 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.`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37689] Improve the comments and variable names for TaskMailbox [flink]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #26469:
URL: https://github.com/apache/flink/pull/26469#discussion_r2046563771
##########
flink-runtime/src/main/java/org/apache/flink/streaming/runtime/tasks/mailbox/TaskMailboxImpl.java:
##########
@@ -96,37 +96,26 @@ public boolean hasMail() {
return !batch.isEmpty() || hasNewMail;
}
- @Override
- public int size() {
- final ReentrantLock lock = this.lock;
- lock.lock();
- try {
- return batch.size() + queue.size();
- } finally {
- lock.unlock();
- }
- }
-
Review Comment:
Move the size() in `TaskMailboxImpl` so that it follows the order of the declaration in the interface `TaskMailbox`.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37689] Improve the comments and variable names for TaskMailbox [flink]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #26469:
URL: https://github.com/apache/flink/pull/26469#discussion_r2046563202
##########
flink-runtime/src/main/java/org/apache/flink/streaming/runtime/tasks/mailbox/TaskMailbox.java:
##########
@@ -138,21 +144,23 @@ public interface TaskMailbox {
*
* @return true if there is at least one element in the batch; that is, if there is any mail at
* all at the time of the invocation.
+ * @throws IllegalStateException if called from non-mailbox thread.
*/
boolean createBatch();
/**
- * 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
Review Comment:
`(head of queue)` -> `(head of batch)`. We only fetch the head mail from the `batch` not the `queue` with `tryTakeFromBatch`.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37689] Improve the comments and variable names for TaskMailbox [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #26469:
URL: https://github.com/apache/flink/pull/26469#issuecomment-2809060176
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "7d4be19cdcb03e619232d2f5f177f8d5611b6f7c",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "7d4be19cdcb03e619232d2f5f177f8d5611b6f7c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 7d4be19cdcb03e619232d2f5f177f8d5611b6f7c UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37689] Improve the comments and variable names for TaskMailbox [flink]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #26469:
URL: https://github.com/apache/flink/pull/26469#issuecomment-2811573420
ping @AHeise @StefanRRichter @JunRuiLee cc @1996fanrui @davidradl @GOODBOY008
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37689] Improve the comments and variable names for TaskMailbox [flink]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #26469:
URL: https://github.com/apache/flink/pull/26469#discussion_r2046562036
##########
flink-runtime/src/main/java/org/apache/flink/streaming/runtime/tasks/mailbox/TaskMailbox.java:
##########
@@ -97,26 +97,32 @@ public interface TaskMailbox {
boolean hasMail();
/**
- * Returns an optional with either the oldest mail from the mailbox (head of queue) if the
- * mailbox is not empty or an empty optional otherwise.
+ * 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.
*
* <p>Must be called from the mailbox thread ({@link #isMailboxThread()}.
*
* @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.
Review Comment:
Supplement the missing exception into comments.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37689] Improve the comments and variable names for TaskMailbox [flink]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #26469:
URL: https://github.com/apache/flink/pull/26469#discussion_r2046560924
##########
flink-runtime/src/main/java/org/apache/flink/streaming/runtime/tasks/mailbox/TaskMailbox.java:
##########
@@ -75,7 +75,7 @@ public interface TaskMailbox {
int MIN_PRIORITY = -1;
/**
- * 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
Review Comment:
`message` -> `mail`. `mail` is a more suitable name in the context of `TaskMailbox`.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org