leerho commented on code in PR #576:
URL: https://github.com/apache/datasketches-java/pull/576#discussion_r1719009320
##########
src/test/java/org/apache/datasketches/quantiles/DirectQuantilesMemoryRequestTest.java:
##########
@@ -47,35 +46,30 @@ public void checkLimitedMemoryScenarios() { //Requesting
application
final int initBytes = ((2 * k) + 4) << 3; //just the BB
//########## Owning Implementation
- // This part would actually be part of the Memory owning implemention so
it is faked here
- try (WritableHandle wdh = WritableMemory.allocateDirect(initBytes,
- ByteOrder.nativeOrder(), new DefaultMemoryRequestServer())) {
- final WritableMemory wmem = wdh.getWritable();
- println("Initial mem size: " + wmem.getCapacity());
-
- //########## Receiving Application
- // The receiving application has been given wmem to use for a sketch,
- // but alas, it is not ultimately large enough.
- final UpdateDoublesSketch usk1 =
DoublesSketch.builder().setK(k).build(wmem);
- assertTrue(usk1.isEmpty());
-
- //Load the sketch
- for (int i = 0; i < u; i++) {
- // The sketch uses The MemoryRequest, acquired from wmem, to acquire
more memory as
- // needed, and requests via the MemoryRequest to free the old
allocations.
- usk1.update(i);
- }
- final double result = usk1.getQuantile(0.5);
- println("Result: " + result);
- assertEquals(result, u / 2.0, 0.05 * u); //Success
-
- //########## Owning Implementation
- //The actual Memory has been re-allocated several times,
- // so the above wmem reference is invalid.
- println("\nFinal mem size: " + wmem.getCapacity());
- } catch (Exception e) {
- throw new RuntimeException(e);
+ // This part would actually be part of the Memory owning implementation so
it is faked here
+ WritableMemory wmem = WritableMemory.allocateDirect(initBytes,
ByteOrder.nativeOrder(), new DefaultMemoryRequestServer());
+ println("Initial mem size: " + wmem.getCapacity());
+
+ //########## Receiving Application
+ // The receiving application has been given wmem to use for a sketch,
+ // but alas, it is not ultimately large enough.
+ final UpdateDoublesSketch usk1 =
DoublesSketch.builder().setK(k).build(wmem);
+ assertTrue(usk1.isEmpty());
+
+ //Load the sketch
+ for (int i = 0; i < u; i++) {
+ // The sketch uses The MemoryRequest, acquired from wmem, to acquire
more memory as
+ // needed, and requests via the MemoryRequest to free the old
allocations.
+ usk1.update(i);
}
+ final double result = usk1.getQuantile(0.5);
+ println("Result: " + result);
+ assertEquals(result, u / 2.0, 0.05 * u); //Success
+
+ //The actual Memory has been re-allocated several times,
+ // so the the wmem reference is invalid. Use the sketch to get the last
memory reference.
+ WritableMemory lastMem = usk1.getMemory();
Review Comment:
The test of success is on line 67. By definition, if line 67 passes, the
memory had to grow larger than the initial size. If the memory capacity did
not grow larger than the initial size, the sketch would run out of memory and
the test would fail. In fact it would fail before it ever reached line 67. So
putting an extra test that the memory in fact grew beyond the initial size is
redundant and impotent because that extra test would either never be executed
or it would always pass.
--
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]
