Copilot commented on code in PR #4498:
URL: https://github.com/apache/cassandra/pull/4498#discussion_r2572644477
##########
src/java/org/apache/cassandra/service/GCInspector.java:
##########
@@ -326,14 +319,43 @@ public double[] getAndResetStats()
private static long getAllocatedDirectMemory()
{
- if (BITS_TOTAL_CAPACITY == null) return -1;
+ long fieldValue = getFieldValue(BITS_TOTAL_CAPACITY_JAVA_8, true);
Review Comment:
The `isAtomicLong` parameter should be `false` for the Java 8 field. In Java
8, `totalCapacity` is a regular `long` field, not an `AtomicLong`. Only in Java
11 was it changed to `TOTAL_CAPACITY` as an `AtomicLong`.
This will cause a `ClassCastException` when running on Java 8, as the code
will attempt to cast a `Long` to `AtomicLong`.
Suggested fix:
```java
long fieldValue = getFieldValue(BITS_TOTAL_CAPACITY_JAVA_8, false);
```
```suggestion
long fieldValue = getFieldValue(BITS_TOTAL_CAPACITY_JAVA_8, false);
```
##########
src/java/org/apache/cassandra/service/GCInspector.java:
##########
@@ -58,34 +59,26 @@ public class GCInspector implements NotificationListener,
GCInspectorMXBean
* The field from java.nio.Bits that tracks the total number of allocated
* bytes of direct memory requires via ByteBuffer.allocateDirect that have
not been GCed.
Review Comment:
Typo: "requires" should be "requested"
```suggestion
* bytes of direct memory requested via ByteBuffer.allocateDirect that
have not been GCed.
```
##########
src/java/org/apache/cassandra/service/GCInspector.java:
##########
@@ -326,14 +319,43 @@ public double[] getAndResetStats()
private static long getAllocatedDirectMemory()
{
- if (BITS_TOTAL_CAPACITY == null) return -1;
+ long fieldValue = getFieldValue(BITS_TOTAL_CAPACITY_JAVA_8, true);
+
+ if (fieldValue == -1)
+ fieldValue = getFieldValue(BITS_TOTAL_CAPACITY_JAVA_11, true);
+
+ return fieldValue;
+ }
+
+ private static Field getField(Class<?> clazz, String fieldName)
+ {
+ try
+ {
+ Field field = clazz.getDeclaredField(fieldName);
+ field.setAccessible(true);
+ return field;
+ }
+ catch (Throwable t)
+ {
+ logger.trace("Error accessing field {} of {}", fieldName,
clazz.getName(), t);
+ // Return null to indicate failure
+ return null;
+ }
+ }
+
+ /**
+ * This method works well with JDK 8/11
Review Comment:
The documentation comment "This method works well with JDK 8/11" is vague
and doesn't explain what the method does or its parameters. Consider improving
it to:
```java
/**
* Retrieves the value of a Field, handling both regular long fields (Java
8) and AtomicLong fields (Java 11+).
*
* @param field the Field to retrieve the value from
* @param isAtomicLong true if the field is an AtomicLong, false if it's a
regular long
* @return the field value, or -1 if retrieval fails
*/
```
```suggestion
* Retrieves the value of a Field, handling both regular long fields
(Java 8) and AtomicLong fields (Java 11+).
*
* @param field the Field to retrieve the value from
* @param isAtomicLong true if the field is an AtomicLong, false if it's
a regular long
* @return the field value, or -1 if retrieval fails
```
##########
src/java/org/apache/cassandra/service/GCInspector.java:
##########
@@ -58,34 +59,26 @@ public class GCInspector implements NotificationListener,
GCInspectorMXBean
* The field from java.nio.Bits that tracks the total number of allocated
* bytes of direct memory requires via ByteBuffer.allocateDirect that have
not been GCed.
*/
- final static Field BITS_TOTAL_CAPACITY;
+ final static Field BITS_TOTAL_CAPACITY_JAVA_8;
+ final static Field BITS_TOTAL_CAPACITY_JAVA_11;
-
static
{
- Field temp = null;
+ Field totalTempFieldJava8 = null;
+ Field totalTempFieldJava11 = null;
Review Comment:
The variable name `totalTempFieldJava8` is inconsistent with the naming
pattern. Consider using `tempFieldJava8` to be consistent (without "total"),
since the final static field name doesn't include "TOTAL" in the middle, and
"Temp" already indicates it's a temporary variable. Alternatively, keep it
consistent with the pattern used for the static field names.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
