Copilot commented on code in PR #7910:
URL: https://github.com/apache/ignite-3/pull/7910#discussion_r3020182766
##########
modules/core/src/main/java/org/apache/ignite/internal/thread/ThreadUtils.java:
##########
@@ -53,6 +53,34 @@ public class ThreadUtils {
/** System line separator. */
private static final String NL = System.lineSeparator();
+ /**
+ * Performs thread dump and prints all available info to the given log
with {@code WARN} or {@code ERROR} logging level depending on
+ * {@code isErrorLevel} parameter.
+ *
+ * @param log Logger.
+ * @param threadId ID of a thread to dump.
+ * @param isErrorLevel {@code true} if thread dump must be printed with
{@code ERROR} logging level, {@code false} if thread dump must
+ * be printed with {@code WARN} logging level.
+ */
+ public static void dumpThread(IgniteLogger log, long threadId, boolean
isErrorLevel) {
+ // We don't really need a full stack, and shorter trace should be less
disruptive I think.
+ int maxStackElements = 15;
+
+ ThreadInfo info =
ManagementFactory.getThreadMXBean().getThreadInfo(threadId, maxStackElements);
+ if (info == null) {
+ return;
+ }
Review Comment:
`dumpThread` calls `ThreadMXBean#getThreadInfo(threadId, ...)` without
validating `threadId`. If callers pass `0`/negative (e.g., sentinel values like
-1 for "no owner"), `getThreadInfo` can throw `IllegalArgumentException`.
Consider returning early when `threadId <= 0` to make this helper safe for
diagnostic use sites.
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/PageHeader.java:
##########
@@ -62,32 +64,32 @@ public class PageHeader {
/** Unknown partition generation. */
static final int UNKNOWN_PARTITION_GENERATION = -1;
- /** 8b Marker/timestamp, 4b Partition generation, 4b flags, 8b Page ID, 4b
Group ID, 4b Pin count, 8b Lock, 8b Temporary buffer. */
- public static final int PAGE_OVERHEAD = 48;
-
/** Marker or timestamp offset. */
private static final int MARKER_OR_TIMESTAMP_OFFSET = 0;
/** Partition generation offset. */
- private static final int PARTITION_GENERATION_OFFSET = 8;
+ private static final int PARTITION_GENERATION_OFFSET =
MARKER_OR_TIMESTAMP_OFFSET + Long.BYTES;
/** Flags offset. */
- private static final int FLAGS_OFFSET = 12;
+ private static final int FLAGS_OFFSET = PARTITION_GENERATION_OFFSET +
Integer.BYTES;
/** Page ID offset. */
- private static final int PAGE_ID_OFFSET = 16;
+ private static final int PAGE_ID_OFFSET = FLAGS_OFFSET + Integer.BYTES;
/** Page group ID offset. */
- private static final int PAGE_GROUP_ID_OFFSET = 24;
+ private static final int PAGE_GROUP_ID_OFFSET = PAGE_ID_OFFSET +
Long.BYTES;
/** Page pin counter offset. */
- private static final int PAGE_PIN_CNT_OFFSET = 28;
+ private static final int PAGE_PIN_CNT_OFFSET = PAGE_GROUP_ID_OFFSET +
Integer.BYTES;
/** Page lock offset. */
- public static final int PAGE_LOCK_OFFSET = 32;
+ public static final int PAGE_LOCK_OFFSET = PAGE_PIN_CNT_OFFSET +
Integer.BYTES;
/** Page temp copy buffer relative pointer offset. */
- private static final int PAGE_TMP_BUF_OFFSET = 40;
+ private static final int PAGE_TMP_BUF_OFFSET = PAGE_LOCK_OFFSET +
OffheapReadWriteLock.LOCK_SIZE;
+
+ /** 8b Marker/timestamp, 4b Partition generation, 4b flags, 8b Page ID, 4b
Group ID, 4b Pin count, 16b Lock, 8b Temporary buffer. */
+ public static final int PAGE_OVERHEAD = PAGE_TMP_BUF_OFFSET + Long.BYTES;
Review Comment:
`PageHeader` layout changed to use `OffheapReadWriteLock.LOCK_SIZE` (now 16
bytes), but the class-level header structure diagram above still describes the
lock data as 8 bytes. Please update the diagram/table to match the new lock
size and resulting `PAGE_OVERHEAD`, to avoid future mistakes when working with
page header offsets.
##########
modules/core/src/main/java/org/apache/ignite/internal/util/OffheapReadWriteLock.java:
##########
@@ -560,6 +589,10 @@ private void awaitCondition(
long passedNanos = System.nanoTime() - startTimeNanos;
if (passedNanos >= timeoutNanos) {
+ long ownerId = getOwnerId(lock);
+
+ ThreadUtils.dumpThread(log, ownerId, true);
Review Comment:
`awaitCondition` unconditionally dumps the thread with `ownerId =
getOwnerId(lock)`, but `ownerId` can be `NO_OWNER_ID` (-1) (e.g., write-lock
timeout caused by readers, or during the clearOwnerId/CAS window). Passing a
non-positive id to `ThreadMXBean#getThreadInfo` may throw
`IllegalArgumentException`, which would mask the intended
`LockTimeoutException` and change timeout behavior. Guard the call (skip dump
when `ownerId <= 0`) and/or make `ThreadUtils.dumpThread` handle non-positive
ids safely.
```suggestion
if (ownerId > 0) {
ThreadUtils.dumpThread(log, ownerId, true);
}
```
##########
modules/core/src/main/java/org/apache/ignite/internal/util/OffheapReadWriteLock.java:
##########
@@ -571,7 +604,8 @@ private void awaitCondition(
"tag", hexInt(tag), false,
"idx", lockIdx, false,
"cond", waitCond.toString(), false,
- "timeout",
TimeUnit.NANOSECONDS.toMillis(timeoutNanos) + "ms", false
+ "timeout",
TimeUnit.NANOSECONDS.toMillis(timeoutNanos) + "ms", false,
+ "ownerId", ownerId, false
));
Review Comment:
The new timeout diagnostic path (reading `ownerId` and dumping a thread)
isn’t covered by existing OffheapReadWriteLock tests (current self-tests use
unlimited timeout). Consider adding a regression test that configures a small
timeout and forces a read/write lock acquisition to time out (both with a
writer owner and with only readers), asserting that `LockTimeoutException` is
thrown without unexpected exceptions.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
