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

Ruby 2.6.4, BigDecimal, and Hydrate error case fixes #415

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Ruby 2.6.4, BigDecimal, and Hydrate error case fixes #415

wants to merge 10 commits into from

Conversation

wflanagan
Copy link
Contributor

This pull request fixes a few things that I've encountered in production.

  1. Hydrate errors.
    The way the lazy record loading was working was causing problems in later versions of Ruby. Specifically, we would get cases where the system would raise "Expected X, received Y" errors. But, when you'd load the record individually, it would work just fine. I'm not 100% clear on what was causing this, but my hunch is that the way this worked before (creating a duplicate array of the results that was indexed, and hydrating on that index) was not working reliably anymore due to performance improvement changes in Ruby.

It's been modified to loop directly over the data itself and hydrate. In our limited testing of this, this seems to have fixed the problem. It also has the side effect of eliminating the double loop of hydrating, and then looping over the result set to see if everything is .loaded?. I also added an array that will give you handle if you get these that you can inspect in development mode. This could be changed to just have true/false and accomplish the same thing.

IMO, what probably should happen, is as soon as any hydrate fails, it should just raise some sort of record not loaded error. It shouldn't bother hydrating the rest of the records if it's just going to throw an error. But, I left it as is for now, in case there's something I don't understand.

  1. BigDecimal.
    I incorporated the move from BigDecimal.new to BigDecimal and removed the if statement.

  2. JSON serializer test errors
    The test for JSON serialization was erroring in production when I tried this with later versions of Ruby. Instead of returning a string or integer representation of TimeUuid columns, it was returning the Cassandra::TimeUuid and this was not serializable by JSON obviously. I used the hook built into ActiveModel to check if the value of a column is a TimeUuid and if so, convert to the hash representation of this column, namely

{n: col_val.to_i, s: col_val.to_s}

and I updated the tests and ensured this was passing.

  1. Dockerfile changes
    I made it so you can build branches with various ruby version by simply changing the ruby version in the docker-compose.yml file. This helps you more quickly flip between versions of Ruby when testing compatibility.

I apologize for the saves from me going back and forth between my dev and production environments. Our database has 1m+ records, and duplicating them into a local test environment to duplicate the error was problematic (basically it didn't work). So, I would investigate in production, then test/revise in my local dev.

This branch is testing green on my local for Ruby 2.6.4 and Ruby 2.5.3. I haven't checked earlier versions of Ruby (or 2.7 preview). We have not pushed this into production at all yet. I'm just going to do that now on my branch and see how it goes. If I run into any big problems, I'll pull the pull request. But, I wanted to get the pull request made to see how this works on earlier versions of ruby and Cassandra, etc. that Travis does.

@wflanagan
Copy link
Contributor Author

@alxgsv Hey, FYI, I ran into a problem on Ruby 2.6.4 with hydrating records. This is the fix for this. You might want to incorporate into your branch. It also has the BigDecimal thing. Thanks for the feedback on your branch (which I used as a base).

@alxgsv
Copy link

alxgsv commented Sep 16, 2019

@wflanagan thank you! Instead of incorporating, I'll wait for your PR to hit master and then happily delete my branch.

@wflanagan
Copy link
Contributor Author

@alxgsv Yeah, I'm looking into 1 case I hit in a million or so queries, but I'd like it to be 100%. So I'd hold off till I give it a go ahead.

@wflanagan
Copy link
Contributor Author

I've pushed the updates and changes from @ilanusse feedback (thanks!).

@wflanagan
Copy link
Contributor Author

Just checking on this. I don't want my work on this to go stale.. and it's easier to make changes when they're somewhat fresh in my mind. I've been using this branch since I pushed forward, and so far feel pretty good about it (no issues that aren't present in the current master as well).

each do |record|
row = rows_by_identity[record.key_values]
record.hydrate(row) if row
track_hydrate_failed << record if record.loaded? == false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you not like:

Suggested change
track_hydrate_failed << record if record.loaded? == false
track_hydrate_failed << record unless record.loaded?

Copy link
Collaborator

@ilanusse ilanusse left a comment

Choose a reason for hiding this comment

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

I've already seen this PR but I thought "might as well reassess". @wflanagan go ahead and merge if you'd like to

@ilanusse
Copy link
Collaborator

ilanusse commented Dec 4, 2019

@wflanagan how would you feel about updating, merging and releasing this? I think it looks production-ready and you've said you've already been running this in a live environment.

@wflanagan
Copy link
Contributor Author

@ilanusse So, I've been running it and it seems fine, no difference in error rates from the last production version of Cequel.

The only pause I'd have is that, in general, I feel like there's some "weirdness" in how the hydrate works when you have maps, sets, etc. in the table. A find doesn't always return these, and I've not run that to ground. Note that the production version does the same thing. But, it's on my todo list to sort out why and fix it, so that a retrieve always returns a record that makes sense.

If you'd like, I can update and then we can merge as well. But, that's the 1 area I've got any reservations about in the code.

@johvet
Copy link

johvet commented Jan 23, 2020

What's preventing this from getting merged?

@wflanagan
Copy link
Contributor Author

wflanagan commented Jan 23, 2020 via email

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