Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[proxima-direct-core] #29 remove Serializable interface from readers and writers #401

Merged
merged 1 commit into from
Aug 14, 2020
Merged

[proxima-direct-core] #29 remove Serializable interface from readers and writers #401

merged 1 commit into from
Aug 14, 2020

Conversation

je-ik
Copy link

@je-ik je-ik commented Jul 21, 2020

No description provided.

@je-ik je-ik changed the title [WIP} [proxima-direct-core] #29 remove Serializable interface from readers and writers [WIP] [proxima-direct-core] #29 remove Serializable interface from readers and writers Jul 21, 2020
@je-ik je-ik force-pushed the 29-remove-serializable-readers-writers branch from 3ac2bef to 53d0bc9 Compare July 21, 2020 17:26
@je-ik je-ik force-pushed the 29-remove-serializable-readers-writers branch from 53d0bc9 to de5df49 Compare July 21, 2020 18:52
@je-ik je-ik force-pushed the 29-remove-serializable-readers-writers branch 8 times, most recently from a889cb9 to c0db138 Compare July 28, 2020 09:18
@je-ik je-ik changed the title [WIP] [proxima-direct-core] #29 remove Serializable interface from readers and writers [proxima-direct-core] #29 remove Serializable interface from readers and writers Jul 28, 2020
@je-ik je-ik force-pushed the 29-remove-serializable-readers-writers branch 2 times, most recently from 1136fdf to 8734c47 Compare July 31, 2020 15:41
@dmvk dmvk self-requested a review August 4, 2020 11:12
dmvk
dmvk reviewed Aug 10, 2020
View reviewed changes
direct/core/src/main/java/cz/o2/proxima/direct/core/BulkAttributeWriter.java Outdated

@SuppressWarnings("unchecked")
@Override
Factory asFactory(RepositoryFactory repositoryFactory);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not big fan of RepositoryFactory being serialized in all reader factories. Why don't we have Factory#create(Repository) instead?

I think we should be aiming for following:

  • RepositoryFactory is only used in top level classes where there is no other way of retrieving repository (eg. source)
  • Lower level factories (readers, ...), should get materialized repository instance, to keep the UDF size low.

I think this may also lead to a cleaner design.

WDYT?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Will try that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the issue with that approach. There are cases in which you need to deserialize the reader/writer without reference to Repository. One example of that is DirectAttributeFamilyDescriptor. I can include RepositoryFactory in the descriptor, but that would mean change in serialVersionUID, because otherwise it would cause NPEs. But we maybe might change it now, because this change is already breaking to serialization. Will try the to go down the path to see if I find any more issues.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't worry about that, because this is a huge breaking change anyway (that should help to avoid future BCs). 👍

@je-ik je-ik force-pushed the 29-remove-serializable-readers-writers branch from 8734c47 to 9ecf3b4 Compare August 10, 2020 11:44
@je-ik je-ik mentioned this pull request Aug 10, 2020
Closed
@je-ik je-ik force-pushed the 29-remove-serializable-readers-writers branch 2 times, most recently from 984d933 to 501a81e Compare August 10, 2020 13:08
@je-ik
Copy link
Author

je-ik commented Aug 12, 2020 •
edited
Loading

All fixed and verified on live pipeline. When approved, I'll squash it.

@je-ik je-ik force-pushed the 29-remove-serializable-readers-writers branch from 07c8a47 to 907f60c Compare August 13, 2020 19:15
@je-ik je-ik requested a review from dmvk August 14, 2020 11:11
@je-ik je-ik force-pushed the 29-remove-serializable-readers-writers branch from 907f60c to b11e42d Compare August 14, 2020 12:13
@je-ik je-ik force-pushed the 29-remove-serializable-readers-writers branch from b11e42d to 2e475bd Compare August 14, 2020 14:31
@sonarcloud
Copy link

sonarcloud bot commented Aug 14, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

83.3% 83.3% Coverage
0.0% 0.0% Duplication

warning The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

@je-ik je-ik force-pushed the 29-remove-serializable-readers-writers branch from 2e475bd to b04cf18 Compare August 14, 2020 15:13
@je-ik je-ik merged commit 87395c0 into master Aug 14, 2020
@je-ik je-ik deleted the 29-remove-serializable-readers-writers branch August 14, 2020 15:13
@je-ik
Copy link
Author

je-ik commented Aug 14, 2020

This closes O2-Czech-Republic#29

@je-ik je-ik mentioned this pull request Aug 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmvk dmvk dmvk approved these changes

Assignees
No one assigned
Labels
beam beam-core core direct direct-core direct-io
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@je-ik @dmvk