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

Protocol Upgrade #13

Open
odobroiu opened this issue Jan 11, 2015 · 5 comments
Open

Protocol Upgrade #13

odobroiu opened this issue Jan 11, 2015 · 5 comments

Comments

@odobroiu
Copy link

Hello @iamaleksey, first of all thanks for this repo. :)

I have started a branch for upgrading to v2 of the protocol since there are a few features I am interested in : https://github.com/odobroiu/seestar/tree/protocol_v2 and am hoping you would be interesting in merging it ( when it is ready), and giving opinions as it goes.

At the beginning I went for the idea of supporting only v2 of the protocol so that I would get more familiar with the code, but am currently thinking of supporting multiple versions.

My current plan is to have all the features from v2 implemented( result pagination, skip_meta, maybe others I did not find yet), then probably do a refactor since it is pretty raw and there are a few things I could think of refactoring, and then maybe go for different versions of the protocol.

Let me know what you think, especially if you feel like certain things should be done differently. :)

@iamaleksey
Copy link
Owner

Thanks. I'll have a look soon.

@odobroiu
Copy link
Author

Based on the idea of " make it, make it work, make it work right, make it work fast" it seems currently all the features in v2 are functional, but the code is pretty much a mess right now :P Will do some refactoring in the next couple of days, and then probably rebase all the commits so far.

One thing I am not sure of is how important is being compatible with the current API is. Currently I do keep a few signatures for the perform/execute methods so that the existing tests run without changes, but am thinking of changing that so that it would be easier( from my current point of view) to use them with the new features.

Either way, I think the best solution would be to discuss about the API/ other open issues after the whole thing is refactored/rebased.

@benghippware
Copy link

Hi,

I've been trying out the code on the protocol_v2 branch:

https://github.com/odobroiu/seestar/tree/protocol_v2

I think the "refactor" commit broke something.

Using a trivial "select count(*)" prepared query and this commit:

odobroiu@b76d6f8

I get meaningful and correct answers ie.

Rows: [[2]]

Then, when I advance to the next commit:

odobroiu@581df1a

I continually get "null" results:

Rows: [[]]

Dumping the "Result" tuple gives:

Result: {rows,{metadata,false,undefined,[]},[[]],undefined}

all the time.

Any thoughts, please?

@odobroiu
Copy link
Author

Hello benghippware,

I found the problem that was causing this, and made a quick fix. Will look more at this to see if I have missed something in the next few days.

Thanks for testing it and pointing this out.

Also, a few weeks ago the plan was to use seestar in a production environment, so having something similar to what datastax offers for the java driver made a lot of sense. Now that has changed, and am not sure how to proceed. I will probably look into matters if they are brought on as fast as I can; and might actually try to add the cluster management, but for sure not in the next several weeks.

@benghippware
Copy link

@odobroiu:

Thanks for the quick response and turnaround.

Thanks for testing it and pointing this out.

No worries. I realise your branch is bleeding edge so sometimes bugs are a part of life.

Now that has changed, and am not sure how to proceed.

No problem. I'm just doing driver evaluations for now and I'm not fully decided on using seestar so please don't feel obligated.

I'm 50/50 between using seestar or CQErl. There doesn't seem to be much comparative information on the Internets so I'm doing some investigations.


On a different note ...

I'd like to suggest that maybe changing the existing API of seestar (which you have done with the "refactor" commit) is something that needs to be planned and managed.

It would make life easier for the downstream developers (like me) if it was (mostly) compatible with existing code.

For example, I much prefer the code before the "refactor" commit because I can just drop it in with minimal changes.

In other words, it's perhaps better to "break many things few times" (ie. a global API change that everyone is warned about) than "break few things many times" (ie. frequent but small API changes that then require changes to production code).

Anyway, this is just an opinion. You might have a different viewpoint.

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