Copilot commented on code in PR #3865:
URL: https://github.com/apache/avro/pull/3865#discussion_r3566792593
##########
lang/java/avro/src/main/java/org/apache/avro/SystemLimitException.java:
##########
@@ -28,13 +28,19 @@
* The following system properties can be set to limit the size of bytes,
* strings and collection types to be allocated:
* <ul>
- * <li><tt>org.apache.avro.limits.byte.maxLength</tt></li> limits the maximum
- * size of <tt>byte</tt> types.</li>
- * <li><tt>org.apache.avro.limits.collectionItems.maxLength</tt></li> limits
the
+ * <li><tt>org.apache.avro.limits.bytes.maxLength</tt> limits the maximum size
+ * of <tt>bytes</tt> types.</li>
+ * <li><tt>org.apache.avro.limits.collectionItems.maxLength</tt> limits the
* maximum number of <tt>map</tt> and <tt>list</tt> items that can be read at
* once single sequence.</li>
Review Comment:
Javadoc grammar: "read at once single sequence" is missing "in a" and reads
incorrectly in generated docs. Please change to "read in a single sequence".
##########
lang/java/avro/src/test/java/org/apache/avro/TestSystemLimitException.java:
##########
@@ -110,6 +111,46 @@ void testCheckMaxStringLength() {
"String length 1024 exceeds maximum allowed");
}
+ @Test
+ void testCheckMaxCollectionAllocation() {
+ // With a small custom limit, cumulative allocations beyond it are
rejected.
+ System.setProperty(MAX_COLLECTION_ALLOCATION_PROPERTY, "1000");
+ resetLimits();
+
+ // Values within the limit pass through and return the running total.
+ assertEquals(0L, checkMaxCollectionAllocation(0L, 0L));
+ assertEquals(1000L, checkMaxCollectionAllocation(0L, 1000L));
+ assertEquals(1000L, checkMaxCollectionAllocation(400L, 600L));
+
+ // A single block over the limit is rejected.
+ SystemLimitException ex = assertThrows(SystemLimitException.class, () ->
checkMaxCollectionAllocation(0L, 1001L));
+ assertTrue(ex.getMessage().contains("exceeds the maximum allowed of
1000"), ex.getMessage());
+
+ // Cumulative blocks that cross the limit are rejected.
+ ex = assertThrows(SystemLimitException.class, () ->
checkMaxCollectionAllocation(600L, 401L));
+ assertTrue(ex.getMessage().contains("exceeds the maximum allowed of
1000"), ex.getMessage());
+
+ // Negative arguments are rejected as malformed.
+ Exception nex = assertThrows(AvroRuntimeException.class, () ->
checkMaxCollectionAllocation(-1L, 10L));
+ assertEquals(ERROR_NEGATIVE, nex.getMessage());
+ nex = assertThrows(AvroRuntimeException.class, () ->
checkMaxCollectionAllocation(10L, -1L));
+ assertEquals(ERROR_NEGATIVE, nex.getMessage());
+
+ // Additive overflow is rejected rather than wrapping to a small value.
+ assertThrows(SystemLimitException.class, () ->
checkMaxCollectionAllocation(Long.MAX_VALUE, 10L));
+ }
+
+ @Test
+ void testCheckMaxCollectionAllocationDefaultsToHeapFraction() {
+ // With no property set, the default is derived from the heap and is well
+ // below Integer.MAX_VALUE on a normally sized JVM, yet generous enough for
+ // legitimate small collections.
+ resetLimits();
+ assertEquals(1024L, checkMaxCollectionAllocation(0L, 1024L));
+ // A pathologically large zero-byte collection is rejected without
allocating.
+ assertThrows(SystemLimitException.class, () ->
checkMaxCollectionAllocation(0L, (long) Integer.MAX_VALUE - 8));
Review Comment:
This assertion can be flaky on very large JVM heaps: the default allocation
cap is derived from `maxMemory()` and then clamped to `MAX_ARRAY_VM_LIMIT`, so
`(long) Integer.MAX_VALUE - 8` may not exceed the computed default limit. Use a
value guaranteed to exceed the cap (e.g., `MAX_ARRAY_VM_LIMIT + 1`) to keep the
test deterministic across environments.
--
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]
