-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Inject execution interceptors using multibinder #24404
Conversation
2ee69e5
to
52c5923
Compare
The GlueMetastoreModule is not truly following the inversion of control paradigm. Many things are created directly in the methods without using injection. Using set multibinder for execution interceptors allows to independently define execution interceptors and use Guice injection. Co-authored-by: Grzegorz Kokosiński <[email protected]>
52c5923
to
ab4fe20
Compare
.setRecordIndividualHttpError(true) | ||
.build().newExecutionInterceptor()) | ||
.addExecutionInterceptor(new GlueHiveExecutionInterceptor(config.isSkipArchive())) | ||
.executionInterceptors(ImmutableList.copyOf(executionInterceptors)) |
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.
You are loosing ordering here. Does it matter?
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.
The set's iteration order is consistent with the binding order. This is convenient when multiple elements are contributed by the same module because that module can order its bindings appropriately. Avoid relying on the iteration order of elements contributed by different modules, since there is no equivalent mechanism to order modules.
So far order is maintained. If we keep adding new elements following the method above is safe. If we add an element in different module, then it could be added in random place (before or after this module).
I have checked the existing interceptors and they are independent from each other, and I believe it is important thing for them. If order would matter then I guess it would have to work as a stack because you have methods there like beforeX
and afterX
.
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.
So I believe order does not matter here.
Inject execution interceptors using multibinder
The GlueMetastoreModule is not truly following the inversion of control
paradigm. Many things are created directly in the methods without using
injection. Using set multibinder for execution interceptors allows to
independently define execution interceptors and use Guice injection.
Co-authored-by: Grzegorz Kokosiński [email protected]