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

Upgrading to Realm Core 0.97.0 #2397

Merged
merged 4 commits into from Mar 9, 2016
Merged

Upgrading to Realm Core 0.97.0 #2397

merged 4 commits into from Mar 9, 2016

Conversation

kneth
Copy link
Member

@kneth kneth commented Mar 8, 2016

@realm/java

@beeender
Copy link
Contributor

beeender commented Mar 8, 2016

Not sure if https://github.com/realm/realm-object-store/pull/52/files#r55271290 this change has impact on us as well. in completedAsyncQueriesUpdate

@kneth
Copy link
Member Author

kneth commented Mar 8, 2016

@beeender The error in the failing test seems to indicate it :-)

@cmelchior
Copy link
Contributor

Should this PR also modify our deleteRealm method? since (all?) the auxillary files are now inlined?

@kneth
Copy link
Member Author

kneth commented Mar 8, 2016

@cmelchior We can remove the aux. files from the list but then removing old database will leave these files untouched. Adding a note to the change log that you must manually remove aux. files is an option.

@cmelchior
Copy link
Contributor

I guess leaving it in for now isn't a big issue.

@@ -169,10 +169,12 @@ public static boolean compact(RealmConfiguration configuration) {
try {
sharedGroup = new SharedGroup(
configuration.getPath(),
SharedGroup.EXPLICIT_TRANSACTION,
SharedGroup.IMPLICIT_TRANSACTION,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

We get a LogicError exception when you try to open the Realm with explicit transaction if it is already opened with implicit transaction. I haven't talked with the core team so I don't know if they are aware of it.

@nhachicha
Copy link
Collaborator

LGTM besides small comments

@beeender
Copy link
Contributor

beeender commented Mar 9, 2016

The TableView's version could be std::numeric_limits<uint64_t>::max() for the default of the source LinkView gets deleted. See realm/realm-core#1510 . After the max uint64_t convert to jlong, it give -1 which is accidentally the same with our RealmResults.TABLE_VIEW_VERSION_NONE. I checked the code, and it seems to be no problem with current implementation. However, @nhachicha would you please help to check again? This can be hard to debug if there is a problem in the future.

@cmelchior
Copy link
Contributor

I think that std::numeric_limits<uint64_t>::max() have always been the value used internally by Core, but now it just got more visible.

@cmelchior
Copy link
Contributor

👍

2 similar comments
@nhachicha
Copy link
Collaborator

👍

@beeender
Copy link
Contributor

beeender commented Mar 9, 2016

👍

@cmelchior cmelchior mentioned this pull request Mar 9, 2016
@stk1m1
Copy link
Contributor

stk1m1 commented Mar 9, 2016

👍

kneth pushed a commit that referenced this pull request Mar 9, 2016
Upgrading to Realm Core 0.97.0
@kneth kneth merged commit 463d811 into master Mar 9, 2016
@kneth kneth deleted the kg/core/0.97.0 branch March 9, 2016 12:35
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants