AVRO-2723: Refactor ReflectData to allow derived classes to customize default values for fields #842

anhldbk · 2020-03-08T09:18:47Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Avro-2723

Tests

No new tests are needed. I just refactor the code base.

Commits

My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does

…Value`

…SchemaDefaultValue`

…hemaDefaultValue()

anhldbk · 2020-03-10T07:28:55Z

@sekikn I tried to fix errors with Travis. Would you please to have a look at this error? https://travis-ci.org/apache/avro/jobs/660406896#L38-L41. I don't know how to troubleshoot it.

sekikn · 2020-03-16T13:25:38Z

That seems an intermittent error and irrelevant to your fix. So just wait for some committer to review your PR or ping them (@Fokko @RyanSkraba could you take a look?). They will rerun the CI if needed.
My comments are as follows:

The PR title should follow the format defined in the PR template. i.e., "AVRO-1234: My Avro PR" (not "Avro 2723: ..."). This will automatically create a link to this PR on the corresponding JIRA issue and makes the git commit summaries consistent.
It may be a matter of personal preference, but with regard to the parameter order, createSchemaDefaultValue(type, field, fieldSchema) feels more natural to me than createSchemaDefaultValue(type, fieldSchema, field), because fieldSchema is derived from field and field is derived from type.

anhldbk · 2020-03-17T02:35:16Z

@sekikn Thank you for your thoughtful reviews. I've updated with the comments.

@Fokko I rebased :D Please have a look

anhldbk · 2020-04-03T08:44:21Z

@sekikn @Fokko ping ....

sekikn · 2020-04-03T12:54:02Z

Your PR looks good to me, but unfortunately I'm just an Avro contributor and not a committer. Sorry 😔

cutting · 2020-04-03T17:17:25Z

The implementation looks reasonable to me. Can you please add a test of this new functionality? Thanks!

anhldbk · 2020-04-06T04:29:05Z

Can you please add a test of this new functionality?

@cutting The refectored code does not change the current logic (all tests are passed). So I think the effective scenario is:

derive class ReflectData
override the new method
assert if things are right

Is that ok?

cutting · 2020-04-06T17:43:28Z

Yes, that is the sort of test I imagine, a subclass that overrides this new method and a test that validates that the overridden method is called in the expected manner and can achieve the desired effects.

anhldbk · 2020-04-09T10:32:07Z

lang/java/avro/src/test/java/org/apache/avro/reflect/TestReflectData.java

+    public int f1 = 55;
+    public String f2 = "a-string";
+    public List<String> f3 = Arrays.asList("one", "two", "three");
+    // public User usr = new User();


@cutting I commented out this field due to an issue with JacksonUtils as described here https://issues.apache.org/jira/browse/AVRO-2775

cutting · 2020-04-09T16:27:34Z

The test looks good. It also looks like an implementation that's generally useful and should probably be included in Avro, not just in a test, no? If we're concerned about back-compatibility, we could implement it optionally, e.g., maybe adding a ReflectData#setGenerateDefaults(boolean) method or something. Is there a reason not to make this the default behavior in some future release?

anhldbk · 2020-04-10T02:15:13Z

@cutting

It also looks like an implementation that's generally useful and should probably be included in Avro, not just in a test, no?

Actually I did implement such things with suggestions from @sekikn. Please have a look at:

the discussion
the actual implementation. I've written several tests against UseInitialValueAsDefault. Some failed to run due to several reasons.

So I decided to put a partial implementation in this PR and gradually resolved my issues.

Is that ok?

cutting · 2020-04-10T18:28:32Z

I was thinking an attribute of ReflectData rather than a subclass might be more useful. Then we might consider changing the default behavior in a future release, since using class-specified defaults seems more natural and expected. As for the implementation, we probably shouldn't be printing stack traces, but rather either rethrowing or silently swallowing them. I don't see the importance of committing the refactoring without the implementation if that's the primary imagined implementation.

…

On Thu, Apr 9, 2020 at 7:15 PM Anh Le (Andy) ***@***.***> wrote: @cutting <https://github.com/cutting> It also looks like an implementation that's generally useful and should probably be included in Avro, not just in a test, no? Actually I did implement such things with suggestions from @sekikn <https://github.com/sekikn>. Please have a look at: - the discussion <https://issues.apache.org/jira/browse/AVRO-2723?focusedCommentId=17054576&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17054576> - the actual implementation <https://github.com/anhldbk/avro-fork/blob/dev/lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectData.java#L103-L195>. I've written several tests against UseInitialValueAsDefault. Some failed to run due to several reasons <https://github.com/anhldbk/avro-fork#why-this-fork>. So I decided to put a partial implementation in this PR and gradually resolved my issues <https://issues.apache.org/jira/browse/AVRO-2785?jql=project%20%3D%20AVRO%20AND%20reporter%20in%20(anhldbk)%20ORDER%20BY%20reporter%20ASC%2C%20created%20DESC> . Is that ok? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#842 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACCC6R6PJYADMLB6D7JEX3RLZ6L5ANCNFSM4LDXVZVA> .

anhldbk · 2020-04-11T04:03:15Z

@cutting

Then we might consider changing the default behavior in a
future release, since using class-specified defaults seems more natural and
expected.

Yep, I like this point.

So to finalize this PR, I'm gonna implement ReflectData#setDefaultsGenerated(boolean) & modify my tests accordingly. Correct?

anhldbk · 2020-04-16T15:48:22Z

@cutting Please response ;)

cutting · 2020-04-16T20:12:46Z

Yes, that sounds like a good plan to me. Thanks.

anhldbk · 2020-04-19T08:04:26Z

@cutting Please have a look at my new commits.

Travis failed with an unrelated error (in Ruby)

anhldbk · 2020-04-21T11:29:44Z

@cutting @Fokko my code is clean now. PTAL.

cutting · 2020-04-21T17:56:56Z

New public & protected methods need javadoc comments. Also, the new fields should probably be private, not protected.

Might the cache be better as WeakHashMap<Class,Object>?

I don't see where isFieldInitialized is called.

- More Java docs - All protected fields are marked as private - Use WeakHashMap instead of HashMap for caching default values

- Rename function `setDefaultValue` into `setDefaultGeneratedValue` to make it clear about the purpose

anhldbk · 2020-04-22T13:41:55Z

@cutting Thank you for your thorough comments. I've fixed them.

lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectData.java

@cutting

- `defaultValues` to use WeakHashMap to friendly with GC - more thorough documentations. Thank you @cutting for point me out

anhldbk · 2020-04-24T06:55:27Z

@cutting I refactored code according your suggestions. Travis complains an issue which is not related to my PR:

Thank you & PTAL.

anhldbk · 2020-04-27T04:21:41Z

@cutting @Fokko would you please have a look at my PR?

anhldbk added 2 commits March 8, 2020 15:28

AVRO-2723: ReflectData to add implementation for `createSchemaDefault…

3fbb6a4

…Value`

AVRO-2723: ReflectData.createSchema() to be refactored to use `create…

1a50d80

…SchemaDefaultValue`

probot-autolabeler bot added the Java Pull Requests for Java binding label Mar 8, 2020

AVRO-2723: applied spotless to beautify code for ReflectData.createSc…

c9c441b

…hemaDefaultValue()

anhldbk changed the title ~~Avro 2723: Refactor ReflectData to allow derived classes to customize default values for fields~~ Avro-2723: Refactor ReflectData to allow derived classes to customize default values for fields Mar 16, 2020

anhldbk changed the title ~~Avro-2723: Refactor ReflectData to allow derived classes to customize default values for fields~~ AVRO-2723: Refactor ReflectData to allow derived classes to customize default values for fields Mar 16, 2020

ReflectData: modified signature for createSchemaDefaultValue

e41db7c

AVRO-2723: provided tests for createSchemaDefaultValue

22e63da

anhldbk commented Apr 9, 2020

View reviewed changes

anhldbk added 4 commits April 19, 2020 11:12

AVRO-2723: implement associated logic for setDefaultsGenerated

74ea897

AVRO-2723: simplified tests for setDefaultsGenerated

2dea3fe

AVRO-2723: add objectToMap to JacksonUtils

1650d10

AVRO-2723: refactor code

3033b90

anhldbk closed this Apr 20, 2020

anhldbk reopened this Apr 20, 2020

AVRO-2723: fixed errors reported by checkstyle

2185dac

anhldbk added 3 commits April 22, 2020 10:27

AVRO-2723: standardize code

21f4060

- More Java docs - All protected fields are marked as private - Use WeakHashMap instead of HashMap for caching default values

AVRO-2723: refactor code

c34ed39

- Rename function `setDefaultValue` into `setDefaultGeneratedValue` to make it clear about the purpose

AVRO-2723: fix format violations reported by checkstyle

878400f

cutting requested changes Apr 22, 2020

View reviewed changes

AVRO-2723: finalize ReflectData

f68f64f

- `defaultValues` to use WeakHashMap to friendly with GC - more thorough documentations. Thank you @cutting for point me out

cutting merged commit cfb6a12 into apache:master Apr 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AVRO-2723: Refactor ReflectData to allow derived classes to customize default values for fields #842

AVRO-2723: Refactor ReflectData to allow derived classes to customize default values for fields #842

anhldbk commented Mar 8, 2020

anhldbk commented Mar 10, 2020

sekikn commented Mar 16, 2020

anhldbk commented Mar 17, 2020

anhldbk commented Apr 3, 2020

sekikn commented Apr 3, 2020

cutting commented Apr 3, 2020

anhldbk commented Apr 6, 2020

cutting commented Apr 6, 2020

anhldbk Apr 9, 2020 •

edited

Loading

cutting commented Apr 9, 2020

anhldbk commented Apr 10, 2020

cutting commented Apr 10, 2020 via email

anhldbk commented Apr 11, 2020

anhldbk commented Apr 16, 2020

cutting commented Apr 16, 2020

anhldbk commented Apr 19, 2020

anhldbk commented Apr 21, 2020

cutting commented Apr 21, 2020

anhldbk commented Apr 22, 2020

anhldbk commented Apr 24, 2020

anhldbk commented Apr 27, 2020

AVRO-2723: Refactor ReflectData to allow derived classes to customize default values for fields #842

AVRO-2723: Refactor ReflectData to allow derived classes to customize default values for fields #842

Conversation

anhldbk commented Mar 8, 2020

Jira

Tests

Commits

Documentation

anhldbk commented Mar 10, 2020

sekikn commented Mar 16, 2020

anhldbk commented Mar 17, 2020

anhldbk commented Apr 3, 2020

sekikn commented Apr 3, 2020

cutting commented Apr 3, 2020

anhldbk commented Apr 6, 2020

cutting commented Apr 6, 2020

anhldbk Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

cutting commented Apr 9, 2020

anhldbk commented Apr 10, 2020

cutting commented Apr 10, 2020 via email

anhldbk commented Apr 11, 2020

anhldbk commented Apr 16, 2020

cutting commented Apr 16, 2020

anhldbk commented Apr 19, 2020

anhldbk commented Apr 21, 2020

cutting commented Apr 21, 2020

anhldbk commented Apr 22, 2020

anhldbk commented Apr 24, 2020

anhldbk commented Apr 27, 2020

anhldbk Apr 9, 2020 •

edited

Loading