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

Wizzardo updates don't read randomNumber #4769

Closed
sebastienros opened this issue May 14, 2019 · 4 comments · Fixed by #4770
Closed

Wizzardo updates don't read randomNumber #4769

sebastienros opened this issue May 14, 2019 · 4 comments · Fixed by #4770

Comments

@sebastienros
Copy link
Contributor

As per https://github.com/TechEmpower/FrameworkBenchmarks/wiki/Project-Information-Framework-Tests-Overview#database-updates on point .vi At least the randomNumber field must be read from the database result set.

From my understanding of the current implementation it seems this value is never read:
https://github.com/TechEmpower/FrameworkBenchmarks/blob/master/frameworks/Java/wizzardo-http/src/main/java/com/wizzardo/techempower/DBController.java#L94

It shouldn't impact the overall performance but I think it's necessary for compliance.

@NateBrady23
Copy link
Member

Agreed; I've opened #4770 -- thanks @sebastienros

@sebastienros
Copy link
Contributor Author

I have looked at 3 implementations so far and none read this field. So now I am questioning my understanding of the requirement.

Do we only need to do a SELECT id, randomNumber or does the randomNumber field actually needs to be read and also assigned to an actual World object?

@NateBrady23
Copy link
Member

NateBrady23 commented May 14, 2019

@sebastienros Yes, they should. Feel free to open new issues or list them here.

I'll be honest; I think it's something we forget to check when merging update implementations.

@sebastienros
Copy link
Contributor Author

ok, thanks for confirming, the doubt had grown ...

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 a pull request may close this issue.

2 participants