-
Notifications
You must be signed in to change notification settings - Fork 827
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
copyTransient is not global #375
Conversation
I understand that you want to have a global setting for copyTransient inside a Kryo instance. But I'm not quite sure about removing the ability to set it on per FieldSerializer basis. I think it should still be possible to change it for a concrete instance of FieldSerializer. Instead, in the constructor of FieldSerializer you could take the initial value for copyTransient from the Kryo instance. Does it make sense to you? |
Yes. @romix That makes sense. I will make those changes and will update the pull request so as to not break compatibility. |
@romix Updated the PR as per your review comments. |
I will add a test case shortly |
@romix Can you please review this PR? |
* NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ | ||
|
||
* NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ | ||
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.
What is this file header touched at all?
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.
Line no: 18, 19 and EOF was missing CRLF. IntelliJ inserted the line feed automatically.
Overall, the PR looks good to me. I've commented on a few indentation issues. May be there are others. Please have a look and fix them. @NathanSweet @magro I think with those small formatting issues fixed, it is OK to merge. Any objections? |
Sorry to bother again. We are looking forward for this feature in Apache Hive. It will be great if we can get this in. |
LGTM. @NathanSweet? |
Allow to set copyTransient globally for all FieldSerializer instances
Fix for #370
There were couple of approaches discussed in mailing list. Making it a config in kryo object seems to be easier.