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.

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

Cast ids to misk.hibernate.Id instead of Long in BulkShardMigrator #1135

Merged
merged 1 commit into from Aug 7, 2019

Conversation

@surrahman
Copy link
Collaborator

commented Aug 7, 2019

We've been seeing ClassCastExceptions in our logs for the BulkShardMigrator: class misk.hibernate.Id cannot be cast to class java.lang.Long (misk.hibernate.Id is in unnamed module of loader 'app'; java.lang.Long is in module java.base of loader 'bootstrap'). This was happening because we were attempting to cast misk.hibernate.Id to a Long. This change corrects that by first casting to a misk.hibernate.Id and then calling .id to get the required Long id.

@surrahman surrahman requested a review from adrw Aug 7, 2019

@surrahman surrahman self-assigned this Aug 7, 2019

@iYung iYung added the bug label Aug 7, 2019

@surrahman surrahman added this to In progress in adrw via automation Aug 7, 2019

.stream()
.map { it as Long }
.collect(Collectors.toSet<Long>())
.resultList

This comment has been minimized.

Copy link
@adrw

adrw Aug 7, 2019

Collaborator

Is resultList short for .list().stream()?

This comment has been minimized.

Copy link
@surrahman

surrahman Aug 7, 2019

Author Collaborator

I believe so .. feels more kotlinish than the clumsy java-style .list().stream().

@adrw

adrw approved these changes Aug 7, 2019

.map { it as Long }
.collect(Collectors.toSet<Long>())
.resultList
.map { (it as Id<*>).id }

This comment has been minimized.

Copy link
@adrw

adrw Aug 7, 2019

Collaborator

Good fix 👍

adrw automation moved this from In progress to Reviewer approved Aug 7, 2019

.stream()
.map { it as Long }
.collect(Collectors.toSet<Long>())
.resultList

This comment has been minimized.

Copy link
@wesleyk

wesleyk Aug 7, 2019

Collaborator

can you fix/align indenting here?

@wesleyk

wesleyk approved these changes Aug 7, 2019

@wesleyk

This comment has been minimized.

Copy link
Collaborator

commented Aug 7, 2019

any test you can add/update corresponding to this fix?

@surrahman surrahman force-pushed the sumair/7aug2019/fix_bulk_shard_migrator_ids branch from 89b1861 to 214fbcc Aug 7, 2019

@surrahman surrahman merged commit 2c3abf2 into master Aug 7, 2019

2 checks passed

ci/circleci: java Your tests passed on CircleCI!
Details
ci/circleci: node Your tests passed on CircleCI!
Details

adrw automation moved this from Reviewer approved to Done Aug 7, 2019

@surrahman

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 7, 2019

We're not sure what's causing this failure .. it happens sometimes and there's nothing in the database or logs to suggest a difference between inputs for successful and failed merges, other than the attached stacktrace. We're going to deploy this and see if it fixes the error. If not, we'll revert and continue investigating.

@surrahman surrahman deleted the sumair/7aug2019/fix_bulk_shard_migrator_ids branch Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.