Skip to content

Resolve all com.google.common.io.Files deprecation warnings#12629

Merged
findepi merged 2 commits intotrinodb:masterfrom
leveyja:leveyja/google-files-deprecation
Aug 3, 2022
Merged

Resolve all com.google.common.io.Files deprecation warnings#12629
findepi merged 2 commits intotrinodb:masterfrom
leveyja:leveyja/google-files-deprecation

Conversation

@leveyja
Copy link
Copy Markdown
Member

@leveyja leveyja commented Jun 1, 2022

Description

Google's Guava's Files#createTempDir and Files.copy and Files.write are all deprecated. This PR removes all usages of these deprecated methods.

Is this change a fix, improvement, new feature, refactoring, or other?

Refactoring

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Removal of deprecated method usages with appropriate replacements. No functional changes should manifest.

How would you describe this change to a non-technical end user or system administrator?

Spring-cleaning ;-)

Related issues, pull requests, and links

Documentation

(:white_check_mark:) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(:white_check_mark:) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

Copy link
Copy Markdown
Contributor

@wendigo wendigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@leveyja leveyja force-pushed the leveyja/google-files-deprecation branch 3 times, most recently from 7d9b9b2 to ab95541 Compare June 2, 2022 07:57
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

Copy link
Copy Markdown
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is red.

Error:  Failures: 
Error:    TestShardRecovery.testShardRecoveryBackupChecksumMismatch:212 expected [/tmp/2732748991945657226/data/storage/c1/f1/c1f1441f-561c-497e-8dc3-d6317ab33dba.orc] but found [/tmp/2732748991945657226/backup/c1/f1/c1f1441f-561c-497e-8dc3-d6317ab33dba.orc]
Error:    TestShardRecovery.testShardRecoveryExistingFileChecksumMismatch:168 expected [/tmp/2903051290351253377/tmp12177291668773958702.tmp] but found [/tmp/2903051290351253377/backup/1a/8b/1a8bd803-f33a-482f-afc5-267d44aa6b78.orc]
Error:    TestShardRecovery.testShardRecoveryExistingFileSizeMismatch:130 expected [/tmp/2569217826565755780/tmp9491014867858533047.tmp] but found [/tmp/2569217826565755780/backup/19/82/19823723-95b6-4ef4-bb72-f72d8276ad7c.orc]

@leveyja
Copy link
Copy Markdown
Member Author

leveyja commented Jun 3, 2022

CI is red.

Error:  Failures: 
Error:    TestShardRecovery.testShardRecoveryBackupChecksumMismatch:212 expected [/tmp/2732748991945657226/data/storage/c1/f1/c1f1441f-561c-497e-8dc3-d6317ab33dba.orc] but found [/tmp/2732748991945657226/backup/c1/f1/c1f1441f-561c-497e-8dc3-d6317ab33dba.orc]
Error:    TestShardRecovery.testShardRecoveryExistingFileChecksumMismatch:168 expected [/tmp/2903051290351253377/tmp12177291668773958702.tmp] but found [/tmp/2903051290351253377/backup/1a/8b/1a8bd803-f33a-482f-afc5-267d44aa6b78.orc]
Error:    TestShardRecovery.testShardRecoveryExistingFileSizeMismatch:130 expected [/tmp/2569217826565755780/tmp9491014867858533047.tmp] but found [/tmp/2569217826565755780/backup/19/82/19823723-95b6-4ef4-bb72-f72d8276ad7c.orc]

Thanks @ebyhr - I switched Files.equals to .equals assuming the content was meant to be equal - File.equals(File) tests the abstract path names are equal (which they're not). I'll fix up those mistakes - cheers for spotting it!

@leveyja leveyja force-pushed the leveyja/google-files-deprecation branch from ab95541 to a9f47bf Compare June 3, 2022 04:57
@leveyja
Copy link
Copy Markdown
Member Author

leveyja commented Jun 3, 2022

1 failing test suite due to timeout - https://github.com/trinodb/trino/runs/6720470622?check_suite_focus=true - will rerun.

@leveyja leveyja force-pushed the leveyja/google-files-deprecation branch from a9f47bf to 145760c Compare June 3, 2022 06:53
@leveyja
Copy link
Copy Markdown
Member Author

leveyja commented Jun 3, 2022

1 check was cancelled(?) re-running

@leveyja leveyja force-pushed the leveyja/google-files-deprecation branch from 145760c to 19ebcf6 Compare June 3, 2022 14:53
@leveyja
Copy link
Copy Markdown
Member Author

leveyja commented Jun 6, 2022

https://github.com/trinodb/trino/runs/6727864278?check_suite_focus=true - failed/cancelled due to timeout (54 minutes running tests). Will re-run...

@leveyja leveyja force-pushed the leveyja/google-files-deprecation branch from 19ebcf6 to eaccc2d Compare June 6, 2022 08:11
@leveyja leveyja force-pushed the leveyja/google-files-deprecation branch from eaccc2d to 548cdaf Compare June 17, 2022 08:13
Copy link
Copy Markdown
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update .mvn/modernizer/violations.xml?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks unrelated. Could you separate a commit or update the commit body?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted, I don't recall removing it (it may have been when I was trying a POC to remove all the warnings, and this got grouped into the com.google ones when I separated the commits).

@leveyja
Copy link
Copy Markdown
Member Author

leveyja commented Jun 18, 2022

Can we update .mvn/modernizer/violations.xml?

Ideally we don't have to update anything, we just need to build w/-Xlint:deprecation and all present and future deprecations will be visible to us (without us updating any xml files). We can combine this w/-Werror to fail the build when any upgrade introduces new deprecated methods (and be warned to change our usages).

Maybe as part of a coordinated plan to remove deprecations adding them to violations.xml would make sense.
See #12628 for how to generate the list of existing deprecations (1300+, 800 of them io.trino / our own usages of our own deprecated methods).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer UTF-8 to be explicit.
i know the Files.writeString uses UTF-8 by default, but providing it explicitly also conveys the intent.

@findepi findepi force-pushed the leveyja/google-files-deprecation branch from 548cdaf to 0e3c8ba Compare August 2, 2022 20:48
@findepi
Copy link
Copy Markdown
Member

findepi commented Aug 2, 2022

(just rebased)

@findepi findepi force-pushed the leveyja/google-files-deprecation branch from 0e3c8ba to fa84928 Compare August 2, 2022 20:49
@findepi findepi force-pushed the leveyja/google-files-deprecation branch from fa84928 to b63a3e9 Compare August 3, 2022 09:04
@findepi findepi merged commit 8d6b82e into trinodb:master Aug 3, 2022
@github-actions github-actions bot added this to the 393 milestone Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants