-
Notifications
You must be signed in to change notification settings - Fork 707
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
Ianoc/configure set converter #1400
Conversation
…igureSetConverter Conflicts: project/Build.scala
|
||
conf.set(ConfigBinaryConverterProvider.ProviderConfKey, ExternalizerSerializer.inj(extern)) | ||
|
||
LzoGenericScheme.setConverter(conv, conf) |
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 we don't want to set the converter here when it's already been set earlier. I'm guessing this is because we want it to be configurable via the new method, before this scheme code is executed?
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.
Exactly, if something else has gotten there first and wanted to set how the deserialization between the two fixed types is done we don't want to override. I'd view this as the default path per say.
Use case so far is that I can swap out different thrift implementations for testing by setting this param
LGTM |
if ((conf.get(ConfigBinaryConverterProvider.ProviderConfKey) == null) || overrideConf) { | ||
val extern = Externalizer(conv) | ||
try { | ||
ExternalizerSerializer.inj.invert(ExternalizerSerializer.inj(extern)).get |
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.
we should move this to ExternalizerSerialization.testRoundTrip(i: Any): Try[Unit]
Allows using the object to set the configuration of which deserializer is being used.