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

nsqd: refactor storage engine #1170

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andyxning
Copy link
Member

@andyxning andyxning commented Jul 26, 2019

Fix #1139
Fix #1137

@andyxning andyxning force-pushed the refactor_nsqd_storage_engine branch 8 times, most recently from 7fbab14 to a593551 Compare July 28, 2019 08:37
@andyxning
Copy link
Member Author

/cc @mreiferson @ploxiln This PR is ready for review. PTAL.

@andyxning andyxning changed the title WIP: refactor nsqd storage engine refactor nsqd storage engine Jul 28, 2019
@andyxning andyxning force-pushed the refactor_nsqd_storage_engine branch from a593551 to 29bd197 Compare July 28, 2019 09:16
@ploxiln
Copy link
Member

ploxiln commented Jul 28, 2019

what's not great about github.com/vmihailenco/msgpack is that it has a bunch of dependencies, including google.golang.org/appengine, and that pulls in github.com/golang/protobuf ...

@andyxning
Copy link
Member Author

andyxning commented Jul 29, 2019

what's not great about github.com/vmihailenco/msgpack is that it has a bunch of dependencies, including google.golang.org/appengine, and that pulls in github.com/golang/protobuf ...

@ploxiln I think it deserves adding the these dependencies since msgpack will make it faster and denser than json or plain bytes. This will store more messages on the same disk and write to disk more messages.

Actually, when the functionality grows, the project will get more and more complex such as logic and dependency. Kubernetes is an example.

@andyxning
Copy link
Member Author

/cc @ploxiln @mreiferson

2 similar comments
@andyxning
Copy link
Member Author

/cc @ploxiln @mreiferson

@andyxning
Copy link
Member Author

/cc @ploxiln @mreiferson

@andyxning
Copy link
Member Author

/cc @jehiah

@andyxning
Copy link
Member Author

/cc @jehiah @mreiferson @ploxiln

1 similar comment
@andyxning
Copy link
Member Author

/cc @jehiah @mreiferson @ploxiln

@andyxning
Copy link
Member Author

/cc @jehiah @mreiferson @ploxiln Anyone can help to take a look at this? It has waited for more than 24 days. :(

@ploxiln
Copy link
Member

ploxiln commented Aug 23, 2019

There are msgpack libraries which do not have dependencies.

While msgpack is more efficient than json, especially for numbers and byte buffers, this is still using strings for keys, instead of a single byte. So, I'm still not a fan of this approach.

@andyxning
Copy link
Member Author

While msgpack is more efficient than json, especially for numbers and byte buffers, this is still using strings for keys, instead of a single byte. So, I'm still not a fan of this approach.

Using single byte for keys will force us to manage parsing keys manually and it is obviously error-prone. This is even make the parsing logic more complicated when a new field added. By using json or msgpack we offload this parsing logic to json or msgpack and they have done quite well in marshal and unmarshal in normal cases and abnormal cases such as some field loss or newly added. We need a tradeoff between maintainability and storage efficiency. In this case, i prefer maintainability.

Btw, @ploxiln thanks for reviewing this. Can you reach @mreiferson @jehiah personally for informing them help to review this PR. This PR has been made for more than 30d. :(

@ploxiln
Copy link
Member

ploxiln commented Aug 26, 2019

@mreiferson recently started at a new job/employer, and is waiting to get official authorization to contribute to NSQ

@andyxning
Copy link
Member Author

OK, let's be more patient. :)

@andyxning
Copy link
Member Author

@mreiferson Ping.

@andyxning
Copy link
Member Author

@ploxiln @mreiferson We should work on this PR for now. :)

@andyxning
Copy link
Member Author

andyxning commented Sep 18, 2019

We need to push this PR forward. @ploxiln Do you know who is also actively working on nsq. We can discuss this on slack or mailing list. Anyway, we need to support this ASAP. This has been delayed by more than one month. :(

@andyxning
Copy link
Member Author

@ploxiln @jehiah @mreiferson Can you please take a look at this.

Internally, i have upgraded a cluster and everything works fine.

@ploxiln
Copy link
Member

ploxiln commented Nov 8, 2019

But you're running a fork of nsq 0.3.8 right? So it doesn't really benefit your organization much if this is merged?

(I'm curious why 0.3.8 - which of the interfaces removed in 1.0 are you dependent on?)

@andyxning
Copy link
Member Author

which of the interfaces removed in 1.0 are you dependent on?

Nothing. We just run 0.3.8 well and nothing new feature in versions after 0.3.8 we need.

@andyxning
Copy link
Member Author

andyxning commented Nov 11, 2019

But you're running a fork of nsq 0.3.8 right? So it doesn't really benefit your organization much if this is merged?

Yes. But i think with this PR nsqd can support more complicated features easily, such as the compression feature.

@mreiferson mreiferson changed the title refactor nsqd storage engine nsqd: refactor storage engine Jun 14, 2020
@yusank
Copy link

yusank commented Jan 20, 2021

Hi, is there any plan to release the new version of nsq with this new storage engine? kind need this feature.

@EvansJahja
Copy link

Hi @ploxiln @jehiah @mreiferson
We would benefit from this feature as well and it seems this has been dragging for over 2 years now. Are there any plans on continuing on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nsqd: refactor NSQD storage backend format
5 participants