ISSUE-9555 fix GcsPinotFS

[lfernandez93]

trying to fix the issue arisen by #9555 changes discussed in slack thread https://apache-pinot.slack.com/archives/C011C9JHN7R/p1664826035484389

[lfernandez93]

cc @zhtaoxiang

[xiangfu0]

cc: @elonazoulay

[codecov-commenter]

Codecov Report

Merging #9556 (f881c3f) into master (8cdae92) will increase coverage by 41.70%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master    #9556       +/-   ##
=============================================
+ Coverage     28.28%   69.99%   +41.70%     
- Complexity       53     4905     +4852     
=============================================
  Files          1917     1939       +22     
  Lines        102594   103675     +1081     
  Branches      15586    15728      +142     
=============================================
+ Hits          29022    72563    +43541     
+ Misses        70735    26009    -44726     
- Partials       2837     5103     +2266     

Flag	Coverage Δ
integration1	25.85% <ø> (-0.25%)	⬇️
integration2	24.45% <ø> (-0.12%)	⬇️
unittests1	67.39% <ø> (?)
unittests2	15.60% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...pache/pinot/core/query/executor/QueryExecutor.java	12.50% <0.00%> (-87.50%)	⬇️
.../operator/blocks/results/MetadataResultsBlock.java	50.00% <0.00%> (-50.00%)	⬇️
...operator/blocks/results/ExceptionResultsBlock.java	75.00% <0.00%> (-25.00%)	⬇️
...operator/blocks/results/SelectionResultsBlock.java	77.77% <0.00%> (-22.23%)	⬇️
...core/common/datatable/DataTableBuilderFactory.java	52.94% <0.00%> (-13.73%)	⬇️
.../operator/blocks/results/DistinctResultsBlock.java	93.75% <0.00%> (-6.25%)	⬇️
...r/helix/SegmentOnlineOfflineStateModelFactory.java	56.43% <0.00%> (-3.95%)	⬇️
...apache/pinot/common/datatable/DataTableImplV3.java	92.94% <0.00%> (-1.77%)	⬇️
...ache/pinot/common/metadata/ZKMetadataProvider.java	67.22% <0.00%> (-0.56%)	⬇️
...rg/apache/pinot/core/minion/RawIndexConverter.java	57.27% <0.00%> (-0.53%)	⬇️
... and 1345 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[walterddr]

could you elaborate more on why this change is needed? thx

[lfernandez93]

hey, gladly, when done with @afterclass this particular test was failing, it's important to note that this test only runs if this is provided

pinot/pinot-plugins/pinot-file-system/pinot-gcs/src/test/java/org/apache/pinot/plugin/filesystem/GcsPinotFSTest.java

Line 84 in 9f92392

String keyFile = System.getenv("GOOGLE_APPLICATION_CREDENTIALS");

given that I made changes around that class I was running this test, and I found that because it was not getting cleaned up after every method this assertion was failing

pinot/pinot-plugins/pinot-file-system/pinot-gcs/src/test/java/org/apache/pinot/plugin/filesystem/GcsPinotFSTest.java

Line 287 in 9f92392

Assert.assertEquals(files.length, count + 2);

given that there are 2 files created by

pinot/pinot-plugins/pinot-file-system/pinot-gcs/src/test/java/org/apache/pinot/plugin/filesystem/GcsPinotFSTest.java

Line 156 in 9f92392

public void testGcs()

that are not cleaned up.

So I decided to run the clean up after each test method is invoked.

[walterddr]

in this case I think we should split this into before/after class and before/after method. it is super weird to have beforeclass then aftermethod.

[lfernandez93]

I don't think a fully follow, what do we need to split in particular? the only thing giving issue is that it was AfterClass so some files that were not cleaned up in the middle were tainting the results of another test, so it's just doing the clean up per method.

[walterddr]

hmm. github doesn't allow me to comment outside of range. can I directly push to your branch?

[walterddr]

just did

[lfernandez93]

seems like everything came out okay! :)

[walterddr]

yeah, the test is skipped b/c there's no credential on GHA. let me verify locally

[lfernandez93]

i did run locally and it works

[lfernandez93]

do we need anyone else review?

[lfernandez93]

0.11.0 version with this fix has been deployed in our clusters, things seem okay.