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

Usecontribdjango #523

Closed
wants to merge 211 commits into from
Closed

Usecontribdjango #523

wants to merge 211 commits into from

Conversation

sum12
Copy link
Contributor

@sum12 sum12 commented Jul 13, 2018

I am not entirely sue this is how the code should be organized.
Or even if the code should even go here.

sum12 and others added 30 commits March 21, 2018 22:02
there could be errors during backup in which case the file handle is left
open. Also there could be errors in closing file, catch and return that
too unless there have been no prior errors.
Both the cases now stand handled.

Closes rqlite#400
Make sure file is correctly closed and catch closing error
This involved adding support for a new Raft Node ID.
Test that query via None works without leader
Currently the CLI doesn’t support Basic Auth credentials. This change adds a new option, `-u, —user` to accept an username and password in the form of `username:password`. Credentials are then used to set the Authorization header for all HTTP requests.

Resolves: rqlite#369
Add Basic Auth credentials support to the CLI
Tighter help message for basic auth creds
Remove any matching node before a new node joins
This addresses the race between opening and the store and starting the
join. It's not robust however, and needs more work.
this registers the extra functions required by the django backend
@alanjds
Copy link

alanjds commented Jul 13, 2018

Well, this is a nice hack.

It will work until Django's implementation changes. Should not occur a lot, but will need to be tracked.

I vote for being accepted after polished a bit, but keep pursuing a way to transfer client-side functions code to serverside.

@otoolep
Copy link
Member

otoolep commented Jul 14, 2018

I need more context here -- I don't really understand what this is about.

Also, I'm unlikely to add support for a particular ORM such as Django. Not ruling it out, but I prefer to keep rqlite focused.

@sum12
Copy link
Contributor Author

sum12 commented Jul 15, 2018

Let me give you more context on why a PR to rqlite.

sqlite3 has a mechanism to define custom functions and use them while running queries.
example: django_extract_year
select * from expenses where django_extract_year(spent_on)=2017

So now I have the list of all the expenses from 2017.

both sqlite3 module from python and gp-sqlite3 have api to register these custom functions.
go-sqlite3 expects param Connection_hook which is a function. The functions receive a param *sqlite.SQLiteCon. This params has a method create_function which can be used register custom functions.

Here is an example of how it is done https://github.com/mattn/go-sqlite3/blob/master/_example/custom_func/main.go.

Because sqlite3 has this ability to have custom functions. There would be other ORMs who would have such custom functions. And to use these ORMs with rqlite one will need those functions.

I agree that rqlite is not necessarily the place to hold these extra functions. But since rqlite uses go-sqlite3, rqlite should have mechanism to either register these functions "Dynamically" (I dont know if that is even possible in golang) and/or have them in contrib(folder?/repo?) and register when asked.

Does that help in understanding or is further confusing ? :-)

@alanjds
Copy link

alanjds commented Jul 16, 2018

I understand the need to "bring the code along" because Golang does not allow dynamic bindings usually, but narrowing to Django needs is wrong.

The Right Way is to have a db-side programming language. But how?

Talking with @sbneto we thought about wire-transfering the code to the servers and compile + link locally, but is hard in Go.

If something like a Lua runtime got embedded, at least the ORM could send its functions ahead of compilation time, without needing the whole compilation toolchain on the server.

@alanjds
Copy link

alanjds commented Jul 16, 2018

Another decision: should this PR be accepted as a keg-leg until a db-side language got integrated, just to support at least one ORM? Is this a good compromise?

I do love to see Django and RQLite working, but I an undecided.

@otoolep
Copy link
Member

otoolep commented Jul 16, 2018

What is a "keg-leg"?

@alanjds
Copy link

alanjds commented Jul 16, 2018

@otoolep: Is a weird word for "handicapped by too much beer last night". Sorry for that.

In the context, the idea is intended to be of "so ugly that should be madness to be merged and will be replaced very soon".

@otoolep
Copy link
Member

otoolep commented Jul 25, 2018

OK, thanks @alanjds

I am planning more major work on rqlite for 5.0 in a couple of weeks or so (busy with other stuff right now). So I am reluctant to merge anything that may not make sense (and this code change isn't ready yet). The changes could be significant, including a brand new consensus module.

@sum12
Copy link
Contributor Author

sum12 commented Jul 25, 2018 via email

@otoolep
Copy link
Member

otoolep commented Jul 25, 2018

I would be open to a contrib directory as you suggest. However any code in there would still need to be high-quality, and I would need an HTTP API exposed that would allow people explicitly hook the stuff in.

The connection model is changing is significantly in 5.0, and since this functionality is based on that, anything you do right now is liable to need serious rework. But the general idea you have should work.

@sum12
Copy link
Contributor Author

sum12 commented Jul 27, 2018 via email

@otoolep
Copy link
Member

otoolep commented Jul 27, 2018

OK, please generate a PR and I'll take a look -- thank you.

@sum12
Copy link
Contributor Author

sum12 commented Aug 3, 2018

@otoolep I have updated the PR.
Do you have any new reviews for me ?

@otoolep
Copy link
Member

otoolep commented Aug 4, 2018

Thanks @sum12

Before I merge anything (not sure if that's what you're suggesting now), a few more things need to happen. Commented-out code needs to be removed, tests need to be added, and a README needs to be added.

Alternatively you can wait until I play with what you have now. However it's going to be 3-4 weeks because I have non-programming commitments right now that mean I don't have time for coding right now. :-/

@tomdesair
Copy link

I also want to use rqlite in a new Django project. Therefor I wanted to check if this PR is still waiting on the 5.0 release or if it could already be merged in a new 4.x release?

Thank you all for all the effort you've put in rqlite and best regards,
Tom

@otoolep
Copy link
Member

otoolep commented Feb 27, 2019

I have no plans to merge this at the moment. Right now the goals for rqlite mean I want to keep it quite focused, and not have any ORM or web-framework -specific features.

@alanjds
Copy link

alanjds commented Mar 1, 2019

I understand, @otoolep. However, please keep in mind that any application needing DB-side custom procedures will not be able to be ported for now.

I am sure you have not spare hands to do everything. It is just a reminder of the consequences of this decision.

@sum12
Copy link
Contributor Author

sum12 commented Mar 7, 2019

@tomdesair you should be able to use rqlite with django without this PR too.

This PR just add the functionality to call custom time.Time based functions. So rqlite should work just fine if you are not using these special functions in your project

@alanjds
Copy link

alanjds commented Mar 7, 2019

@tomdesair you should be able to use rqlite with django without this PR too.

I cannot assure that. Without this PR the ORM of Django does not pass the tests for the SQLite backend. You may be able to do something, but YMMV.

@otoolep
Copy link
Member

otoolep commented Nov 30, 2019

v5 development is now highly speculative, it may not continue in its current form.

Thank you for your contribution though.

@otoolep otoolep closed this Nov 30, 2019
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.

6 participants