blaginin commented on code in PR #16145:
URL: https://github.com/apache/datafusion/pull/16145#discussion_r2102140935
##########
datafusion/execution/src/memory_pool/pool.rs:
##########
@@ -452,64 +453,64 @@ mod tests {
let mut r1 = MemoryConsumer::new("unspillable").register(&pool);
// Can grow beyond capacity of pool
r1.grow(2000);
- assert_eq!(pool.reserved(), 2000);
+ assert_snapshot!(pool.reserved(), @"2000");
let mut r2 = MemoryConsumer::new("r2")
.with_can_spill(true)
.register(&pool);
// Can grow beyond capacity of pool
r2.grow(2000);
- assert_eq!(pool.reserved(), 4000);
+ assert_snapshot!(pool.reserved(), @"4000");
Review Comment:
IMO this doesn’t need to be a snapshot because `4000 isn’`t prone to
formatting errors. For example, you snapshot errors below, which makes sense
since it allows updating messages and error structures easily. But here it’s
just a const representing test setup.
##########
datafusion/execution/src/memory_pool/pool.rs:
##########
@@ -586,42 +577,28 @@ mod tests {
let mut r1 = new_consumer_same_name.register(&pool);
// TODO: the insufficient_capacity_err() message is per reservation,
not per consumer.
// a followup PR will clarify this message "0 bytes already allocated
for this reservation"
- let expected = format!("Additional allocation failed with top memory
consumers (across reservations) as:\n foo#{}(can spill: false) consumed 10.0
B,\n foo#{}(can spill: false) consumed 0.0 B.\nError: Failed to allocate
additional 150.0 B for foo with 0.0 B already allocated for this reservation -
90.0 B remain available for the total pool", r0.consumer().id(),
r1.consumer().id());
let res = r1.try_grow(150);
- assert!(
- matches!(
- &res,
- Err(DataFusionError::ResourcesExhausted(ref e)) if
e.to_string().contains(&expected)
- ),
- "should provide proper error for 2 consumers, instead found
{res:?}"
- );
+ assert!(res.is_err());
+ let error = res.unwrap_err();
+ assert_snapshot!(error.to_string(), @"Resources exhausted: Additional
allocation failed with top memory consumers (across reservations) as: foo#0(can
spill: false) consumed 10 bytes, foo#1(can spill: false) consumed 0 bytes.
Error: Failed to allocate additional 150 bytes for foo with 0 bytes already
allocated for this reservation - 90 bytes remain available for the total pool");
// Test: will accumulate size changes per consumer, not per reservation
r1.grow(20);
- let expected = format!("Additional allocation failed with top memory
consumers (across reservations) as:\n foo#{}(can spill: false) consumed 20.0
B,\n foo#{}(can spill: false) consumed 10.0 B.\nError: Failed to allocate
additional 150.0 B for foo with 20.0 B already allocated for this reservation -
70.0 B remain available for the total pool", r1.consumer().id(),
r0.consumer().id());
+
let res = r1.try_grow(150);
- assert!(
- matches!(
- &res,
- Err(DataFusionError::ResourcesExhausted(ref e)) if
e.to_string().contains(&expected)
- ),
- "should provide proper error for 2 consumers(one foo=20.0 B,
another foo=10.0 B, available=70.0 B), instead found {res:?}"
- );
+ assert!(res.is_err());
+ let error = res.unwrap_err();
+ assert_snapshot!(error.to_string(), @"Resources exhausted: Additional
allocation failed with top memory consumers (across reservations) as: foo#1(can
spill: false) consumed 20 bytes, foo#0(can spill: false) consumed 10 bytes.
Error: Failed to allocate additional 150 bytes for foo with 20 bytes already
allocated for this reservation - 70 bytes remain available for the total pool");
// Test: different hashed consumer, (even with the same name),
// will be recognized as different in the TrackConsumersPool
let consumer_with_same_name_but_different_hash =
MemoryConsumer::new(same_name).with_can_spill(true);
let mut r2 =
consumer_with_same_name_but_different_hash.register(&pool);
- let expected = format!("Additional allocation failed with top memory
consumers (across reservations) as:\n foo#{}(can spill: false) consumed 20.0
B,\n foo#{}(can spill: false) consumed 10.0 B,\n foo#{}(can spill: true)
consumed 0.0 B.\nError: Failed to allocate additional 150.0 B for foo with 0.0
B already allocated for this reservation - 70.0 B remain available for the
total pool", r1.consumer().id(), r0.consumer().id(), r2.consumer().id());
Review Comment:
I think it changes the logic of the test slightly because before it'd
accomodate for variable consumer ids but now those are hardcoded in your tests.
This is how I solved this problem before but feel free to explore other options
https://github.com/apache/datafusion/blob/cb45f1f9cc4f4041b8dcf9c7f0f5d51470d5f5a3/datafusion-cli/tests/cli_integration.rs#L33
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
