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

3.0.1 introduces build-breaking API changes, needs a proper version number #419

Closed
rayray opened this issue Jul 28, 2017 · 4 comments
Closed

Comments

@rayray
Copy link

rayray commented Jul 28, 2017

If this project is following semver, then a patch update should not remove or change existing APIs.

If the intent was to introduce breaking changes in a x.y.1 update, then why have an x.y.z version at all???

Not only that, the changes are pretty significant. Might be worth bumping to 4.x.y if the underlying cause is that important.

And finally, none of these significant changes are outlined in the CHANGELOG.

@rayray rayray changed the title 3.0.1 introduces breaking API changes 3.0.1 introduces build-breaking API changes Jul 28, 2017
@rayray rayray changed the title 3.0.1 introduces build-breaking API changes 3.0.1 introduces build-breaking API changes, needs a proper version number Jul 28, 2017
@chrisballinger
Copy link
Contributor

What are the breaking changes? I had no problem going from 3.0.0 to 3.0.1, but there were a few big ones from 2.x -> 3.x.

@rayray
Copy link
Author

rayray commented Jul 28, 2017

Not sure about other methods/properties, but in this commit, this method in 3.0.0:

- (BOOL)registerExtension:(YapDatabaseExtension *)extension
                 withName:(NSString *)extensionName
               connection:(nullable YapDatabaseConnection *)connection;

was renamed to:

- (BOOL)registerExtension:(YapDatabaseExtension *)extension
                 withName:(NSString *)extensionName
                   config:(nullable YapDatabaseConnectionConfig *)config;

API renames/removals should be indicated by a point update (in semver land).

I have to stick with 3.0.0 for now, while I figure out how to get the same behavior.

@chrisballinger
Copy link
Contributor

Ah, I wasn't using that feature. It was removed in 3.0.1 because it wasn't safe.. you probably want to migrate.

@robbiehanson
Copy link
Contributor

robbiehanson commented Jul 29, 2017

Thank you for your honesty @rayray. You're right. This project should follow semver, and should NOT "introduce breaking changes in a x.y.1 update". We'll try our best not to do this again.

And finally, none of these significant changes are outlined in the CHANGELOG.

Yup, I forgot that too. I've now updated the changelog with information on the new version.

Regarding the breaking API changes:

It was discovered that registering extensions with your own custom YDBConnection was unsafe, and could lead to deadlock. Upon analysis, it was determined that there was no safe way to fix the API, as it was simply broken.

If you're using that API, I wouldn't be surprised if you've experienced occasional deadlock when launching the app. In fact, if you're like me, you probably blamed it on Xcode and simply clicked the Run button again...

I have to stick with 3.0.0 for now, while I figure out how to get the same behavior.

The motivation for the original API was performance concerns regarding extension registration. In particular, if you were registering a YDBView (for the first time, or after updating its version), then it could thrash if your database is big enough. For example, the YDBView is trying to sort 100,000 items, using a cache of only 250 objects. The solution was to increase the objectCacheLimit for the connection performing the registration. However, our initial implementation of this solution was to allow you to pass your own connection. We've now realized this is unsafe. And the new API allows you to pass a YapDatabaseConnectionConfig object, which allows you to configure the cache size that will be used by the internal YDBConnection that performs the registration. Thus, this achieves the same performance optimization, but in a different way.

If you have a different reason for using the old API, please let me know. (There's a good chance you're not the only person doing this. So I'd like to address these issues before I get more bug reports...)

That being said, I do recommend you upgrade to v3.0.1. Not only because of the deadlock issue, but also because of the checkpointing improvements it includes. (Mentioned in changelog.)

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

No branches or pull requests

3 participants