-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the ability to intercept File IO by specifying a wrapper class #155
Conversation
polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java
Outdated
Show resolved
Hide resolved
polaris-service/src/main/java/io/polaris/service/catalog/FileIOWrapperFactory.java
Outdated
Show resolved
Hide resolved
polaris-core/src/main/java/io/polaris/core/monitor/MetricRegistryAware.java
Outdated
Show resolved
Hide resolved
@collado-mike I changed it to just be FileIOFactory and added MetricRegistryAware to be consistent with the other xAware interfaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Minor suggestion for an additional metric and add a javadoc and we'll be good
polaris-service/src/test/java/io/polaris/service/catalog/MeasuredFileIOFactory.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public void deleteFile(InputFile file) { | ||
io.deleteFile(file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
measure files deleted? Seems like a useful metric (especially if it spikes real high real fast 😅)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some testing for measuring deleted files. Originally I left it out as MeasuredFileIOFactory is just a test class but I may as well add it as an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh! didn't realize this was a test class. It seems super useful to me. Maybe move it into src/main
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation isn't that great since it doesn't clean up IOs and its metrics may not be super useful, it was just intended to be super simple to confirm that File IO is wrapped properly.
polaris-service/src/main/java/io/polaris/service/config/PolarisApplicationConfig.java
Outdated
Show resolved
Hide resolved
@@ -60,6 +62,16 @@ public MetaStoreManagerFactory getMetaStoreManagerFactory() { | |||
return metaStoreManagerFactory; | |||
} | |||
|
|||
@JsonProperty("io") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, this is redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
factoryType
could be a top level config, but I made it io => factoryType
to push a grouping of io-related configs. For example if you choose to make a metrics-emitting implementation of FileIOFactory, you may want some further configs.
92c2539
to
55b34b8
Compare
55b34b8
to
f56f773
Compare
b863463
to
7b92ae1
Compare
baccf83
to
5ed91cc
Compare
a6f6846
to
434e1e4
Compare
Description
This PR adds the ability to provide wrappers around File IO by specifying a wrapper factory. It comes with a no-op wrapper factory which performs no wrapping. I did not implement any specific type of wrapper as there can be a separate discussion on which interceptors would be useful to put in the repo if any, but it should be uncontroversial that the ability to intercept this is generally useful for use cases such as metrics.
Type of change
How Has This Been Tested?
testFileIOWrapper
.Checklist: