Skip to content

GEODE-5800: remove redundant casts for DataSerializer.readObject()#2546

Closed
galen-pivotal wants to merge 3 commits intoapache:developfrom
galen-pivotal:feature/GEODE-5800
Closed

GEODE-5800: remove redundant casts for DataSerializer.readObject()#2546
galen-pivotal wants to merge 3 commits intoapache:developfrom
galen-pivotal:feature/GEODE-5800

Conversation

@galen-pivotal
Copy link
Contributor

and remove unused methods on InternalDataSerializer.

Copy link
Member

@PurelyApplied PurelyApplied left a comment

Choose a reason for hiding this comment

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

+1 for code cleanup, obviously pending merge conflict resolution and failed test examination. I'm not confident in how AnalyzeSerializablesJUnitTest desire to not break backwards compatibility works in practice.

It also looks like we've let this PR get stale enough that the test failures are a bit strange. It looks like StressNewTest failures were UnitTest failures, but didn't fail in Build. But Build has been replaced by a dedicated UnitTest job in these past three weeks. Still, somewhat concerning that the stress went red where the Build didn't.

Copy link
Contributor

@onichols-pivotal onichols-pivotal left a comment

Choose a reason for hiding this comment

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

looks like good clean-up and makes things much more readable

@jake-at-work
Copy link
Contributor

@galen-pivotal What you waiting for to merge this change?

* Cleanup DataSerializer
* Remove unneeded types
* Remove unneeded casts
* Remove unused code
@galen-pivotal
Copy link
Contributor Author

I was waiting on GEODE-6036 (#2997) before fixing sanctioned serializables. I haven't pushed that up yet but will in a minute.

This will make the test's output anything close to debuggable. I suspect
that the test is failing due to its use of randomness but would like to
verify that.
@galen-pivotal
Copy link
Contributor Author

I'm going to close this until I have more time to devote to cleaning up tests, etc. around this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants