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

drop renaming of zone id #81

Merged
merged 1 commit into from
Sep 19, 2018
Merged

drop renaming of zone id #81

merged 1 commit into from
Sep 19, 2018

Conversation

mkreilly
Copy link

this crashes and @conorhenley said he has been commenting it out. @fscottfoti can we drop this line because the problem in orca has now been fixed? and all the comments above it? or?

this crashes and can be commented out apparently..
@fscottfoti
Copy link
Collaborator

OK, it took a little thinking but I think this is what is going on.

zone_id_x probably doesn't exist because of this commit UDST/orca@614b346 I made in April of 2017. Basically we make sure that when two tables are joined that have the same column, one of them keeps the original column name instead of being _x and _y. Thus I think the line can be removed.

I do still think the core issue is lurking having to do with the cache, as this issue is still open and the default caching hasn't changed. UDST/orca#16 I think it's probably ok for reasons too complex to go into here, but I'd be wary.

@mkreilly
Copy link
Author

Thanks for pondering this! I will merge in the change but leave it as commented out with the comments above as a reminder that there is this issue lurking in the code :(

@mkreilly mkreilly closed this Sep 19, 2018
@mkreilly mkreilly reopened this Sep 19, 2018
@mkreilly mkreilly merged commit 636713c into master Sep 19, 2018
@theocharides
Copy link

@mkreilly I'm just now catching up with the issues/comments on this. SO, commenting that line out is what made our jobs totals low during Horizon. The code returns back to its old behavior with null zone_ids, because we're not using zone_id_x which is unaffected by the cache issue. When you add the line back in, the problem was fixed on my machine, however it crashed when trying to run on the server (as @conorhenley was experiencing).

The crash is due to the update to merge_tables that @fscottfoti mentions. The server must have an updated version of orca, so it didn't work. I've had to add lines that force zone_id_x in so merge_tables can't get rid of it, in order to return to using it as a way of circumventing the cache issue. That is still an open issue: UDST/orca#16

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.

3 participants