Core: Fix negation in requireStrictCleanup

[Fokko]

Found a regression that the snapshots are still around after the commit fails: trinodb/trino#19188

io.trino.plugin.iceberg.catalog.file.TestIcebergFileMetastoreCreateTableFailure.testCreateTableFailureMetadataCleanedUp; (took: 0.0 seconds)
2023-09-28T18:24:02.9837549Z java.lang.AssertionError: [Metadata file should not exist] 
2023-09-28T18:24:02.9837847Z Expecting actual:
2023-09-28T18:24:02.9838206Z   /tmp/test_iceberg_create_table_failure482309508502886299/test_create_failure_4huakfy1pd/metadata
2023-09-28T18:24:02.9838580Z to be an empty directory but it contained:
2023-09-28T18:24:02.9839258Z   [/tmp/test_iceberg_create_table_failure482309508502886299/test_create_failure_4huakfy1pd/metadata/.snap-905595869249987831-1-7e5e14a9-79ec-449d-abf4-9cae0d693663.avro.crc,

This is a result of: #8599 It was actually enabled before, but now it is disabled.

After the fix, I checked locally, the test passes:

[aokolnychyi]

Let me check this one in a bit.

[aokolnychyi]

Hm, isn't the existing logic correct? We should not clean up in strict mode unless it is CleanableFailure? Is there a chance the test in Trino produced a commit error that we treat as unknown or unsafe to clean up?

[aokolnychyi]

I think the one that failed in Trino was testing SchemaNotFoundException here.

[Fokko]

@aokolnychyi Thanks, I'm not 100% sure if it is a regression or intended. A test is failing that seems to make sense. I can take a look tomorrow with fresh eyes, but another set of eyes is very much welcome. Feel free to take over the PR.

[aokolnychyi]

cc @rdblue @amogh-jahagirdar

[rdblue]

I don't think this change is correct. The logic should continue to be to clean up if strict cleanup is not enabled.

However, there is a behavior change here that is probably being hit by this test. We changed behavior and enabled strict cleanup for tables unless it is turned off by the catalog. This may leak files, but it is much safer and avoids table corruption on unknown exceptions.

[Fokko]

I don't think this change is correct. The logic should continue to be to clean up if strict cleanup is not enabled.

I think it is a difference in understanding, if it is strict in cleaning up the files, I would expect it to clean it up. Anyway, good that we have the test and it is expected.