Skip to content

feat: update handler map when field descriptor hashcode changes #13

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
Merged
Meghajit merged 9 commits into from
May 4, 2023
Merged

feat: update handler map when field descriptor hashcode changes #13

Meghajit merged 9 commits into from
May 4, 2023

Conversation

@Meghajit
Copy link
Collaborator

@Meghajit Meghajit commented Apr 27, 2023

Issue

In the dagger repository, the TypeHandlerFactory class maintains a static hash map, which memoizes which handler to use for serialization/deserialization for every proto full name. The type handler objects are created once using the field descriptor and saved as the value of this hash map.

The hypothesis was that this map was being persisted in the memory of a task manager, even when the job terminates or restarts. As a result the handler objects get reused and at the time of serialization when this method is called in the stacktrace:

at com.google.protobuf.DynamicMessage$Builder.verifyContainingType(DynamicMessage.java:619)

the equals comparison fails and hence serialization also fails.

Fix

The fix is then to update the handler corresponding to the proto full name in the hash map, whenever the field descriptor hashcode is a new one.

@Meghajit Meghajit added the bug Something isn't working label Apr 27, 2023
@Meghajit Meghajit self-assigned this Apr 27, 2023
prakharmathur82
prakharmathur82 reviewed Apr 27, 2023
View reviewed changes
dagger-common/build.gradle Outdated
testImplementation 'com.google.protobuf:protobuf-java-util:3.5.0'
testImplementation 'org.grpcmock:grpcmock-junit5:0.5.0'
testImplementation 'org.powermock:powermock-module-junit4:2.0.0-beta.5'
testImplementation 'org.powermock:powermock-api-mockito2:2.0.0-beta.5'
Copy link

@prakharmathur82 prakharmathur82 Apr 27, 2023

Choose a reason for hiding this comment

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

let's not use powermock if possible, it messes up the classpaths

Copy link
Collaborator Author

@Meghajit Meghajit May 3, 2023

Choose a reason for hiding this comment

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

Removed

prakharmathur82
prakharmathur82 reviewed Apr 27, 2023
View reviewed changes
...on/src/test/java/com/gotocompany/dagger/common/serde/typehandler/TypeHandlerFactoryTest.java Outdated
@Test
public void shouldReturnDifferentCopiesOfHandlerObjectWhenFieldDescriptorFullNameIsSameButHashCodeIsDifferent() {
Descriptors.FieldDescriptor mapFieldDescriptor1 = TestBookingLogMessage.getDescriptor().findFieldByName("metadata");
Descriptors.FieldDescriptor mapFieldDescriptor2 = PowerMockito.mock(Descriptors.FieldDescriptor.class);
Copy link

@prakharmathur82 prakharmathur82 Apr 27, 2023

Choose a reason for hiding this comment

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

if you again create this, how the hashcode will be same?

Copy link
Collaborator Author

@Meghajit Meghajit May 3, 2023

Choose a reason for hiding this comment

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

Changed to use protobuf libraries

@Meghajit Meghajit requested a review from sumitaich1998 May 4, 2023 06:06
@Meghajit Meghajit changed the base branch from main to goto-stencil-auto-refresh-2 May 4, 2023 06:08
@Meghajit Meghajit merged commit 10a79d7 into goto-stencil-auto-refresh-2 May 4, 2023
sumitaich1998 added a commit that referenced this pull request May 23, 2023
@sumitaich1998 @Meghajit @lavkesh
feat: stencil schema auto refresh and fix typehandler bug(#9)
b7338ad 
* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* test: fix tests

* test: fix tests

* test: fix tests

* test: fix tests

* test: fix tests

* test: fix tests

* test: fix tests

* test: fix tests

* test: fix tests

* test: fix tests

* test: fix tests

* test: fix tests

* feat: auto stencil schema refresh

* feat: auto stencil schema refresh

* feat: auto stencil schema refresh

* feat: auto stencil schema refresh

* feat: auto stencil schema refresh

* feat: auto stencil schema refresh

* feat: auto stencil schema refresh

* feat: auto stencil schema refresh

* feat: auto schema refresh

* feat: stencil auto refresh

* feat: stencil auto refresh

* feat: stencil auto refresh

* feat: stencil auto refresh

* feat: stencil auto refresh

* feat: stencil auto refresh

* feat: stencil auto refresh

* tests: fix tests

* fix: clear typehandlermap on schema update

* fix: clear typehandlermap on schema update

* fix: schema refresh strategy

* fix: schema refresh strategy

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* feat: enable stencil auto schema refresh

* tests: add tests for auto schema refresh

* test: add tests for auto schema refresh

* test: add tests for auto schema refresh

* test: add tests for auto schema refresh

* test: add tests for auto schema refresh

* refactor: rename transformFromProtoMap() to transformFromProtoUsingCache()

* refactor: rename transformFromProtoMap() to transformFromProtoUsingCache()

* refactor: rename transformFromProtoMap() to transformFromProtoUsingCache()

* refactor: rename transformFromProtoMap() to transformFromProtoUsingCache()

* refactor: rename transformFromProtoMap() to transformFromProtoUsingCache()

* refactor: rename transformFromProtoMap() to transformFromProtoUsingCache()

* refactor: rename transformFromProtoMap() to transformFromProtoUsingCache()

* refactor: rename transformFromProtoMap() to transformFromProtoUsingCache()

* refactor: rename transformFromProtoMap() to transformFromProtoUsingCache()

* refactor: rename transformFromProtoMap() to transformFromProtoUsingCache()

* refactor: rename transformFromProtoMap() to transformFromProtoUsingCache()

* refactor: rename transformFromProtoMap() to transformFromProtoUsingCache()

* test: add tests for auto schema refresh

* fix: remove caching for nested types

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* feat: auto schema refresh

* docs: add configs for auto schema refresh

* docs: configs for auto schema refresh

* refactor: change stencil config defaults

* refactor: change stencil config defaults

* refactor: change stencil config defaults

* fix: remove non-serializable stencilconfig

* tests: fix tests

* tests: fix tests

* tests: fix tests

* tests: fix tests

* feat: update handler map when field descriptor hashcode changes (#13)

* chore: move stencil back to dependency

* chore: version bump of dependencies

* chore: version issues

* feat: update handler map when field descriptor hashcode changes

- add mockito extension to mock final classes ( ref: https://www.baeldung.com/mockito-final)

[https://go-jek.atlassian.net/browse/DSTRM-1379]

* feat: remove dead code and fix checkstyle issues

[https://go-jek.atlassian.net/browse/DSTRM-1379]

* feat: use PowerMockito instead of mockito plugin

- add test dependency of powermockito in dagger-common

* feat: remove dependency of power-mockito and use protobuf libs for test

[https://go-jek.atlassian.net/browse/DSTRM-1379]

---------

Co-authored-by: lavkesh <[email protected]>

* fix: dont clear typehandlermap

* fix: fix checkstyle

* fix: revert cleartypehandler to protected

---------

Co-authored-by: Meghajit Mazumdar <[email protected]>
Co-authored-by: lavkesh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sumitaich1998 sumitaich1998 sumitaich1998 approved these changes

+1 more reviewer

@prakharmathur82 prakharmathur82 prakharmathur82 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Meghajit Meghajit

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Meghajit @prakharmathur82 @sumitaich1998 @lavkesh