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

Discuss: BEP-10 #38

Closed
ldmberman opened this issue May 7, 2018 · 16 comments
Closed

Discuss: BEP-10 #38

ldmberman opened this issue May 7, 2018 · 16 comments
Labels
discuss existing BEP Use this issue like a forum for discussion a specific BEP that already exists

Comments

@ldmberman
Copy link
Contributor

No description provided.

@ldmberman
Copy link
Contributor Author

I have certain concerns about the motivation of the BEP.

c) there is data duplication between LevelDB and MongoDB

We might want to investigate how to replace Tendermint's storage backend. It feels big and probably deserves a separate BEP. It might involve:

  • talking to the Tendermint team about abstracting away storage backends;
  • investigate the performance trade off; at first I was thinking LevelDB is hard to beat but the new MongoDB's backend shows off some very promising numbers;
  • not lose Mongo's rich indexing capabilities.

b) the current design makes some calls going from BigchainDB to Tendermint to BigchainDB again, penalizing performance

Approaching A is likely worse performance-wise compared to the tcp connections created in advance for ABCI. Do you consider Approach B to be more complicated even if we rewrite ABCI method by ABCI method keeping the not yet migrated ABCI calls go through py-abci?

e) the logic to validate transactions is spread across multiple files and it's difficult to manage, and a heavy refactoring is needed

What prevents us from consolidating this logic independently from this BEP?

a) managing multiple services together adds overhead and complicates deployment

What are the exact issues we face here? If we have only 2 processes (mongo and bigchaindb-tendermint), would it make it better?

f) the configuration is spread across BigchainDB and Tendermint.

Do we have some rough ideas about how to unify the configuration? Should it be a different PEP?

In general, getting rid of py-abci comes at a cost of maintaining our own integration with Tendermint. The integration itself should be faster and easier to maintain. The reimplemented transaction validation logic which is tightly coupled to the integration itself will be also faster but subjectively harder to maintain since Go is new to the team and offers fewer of the high-level programming language features.

@ttmc
Copy link
Contributor

ttmc commented May 7, 2018

This BEP starts out by listing several issues with the current situation, but it doesn't try (or claim to try) to solve them all in one BEP.

Replacing LevelDB with MongoDB would definitely be the topic of a future and separate BEP, if we ever do it at all. The process of writing the BEP would help us weigh the pros and cons. Right now, it's not obvious to me that using MongoDB-only would be better.

The refactoring of the transaction validation code will also be handled by other BEPs, starting with BEP-9. For now, we'll just add a new HTTP API endpoint that the Go code can call to ask BigchainDB to check the validity of the passed transaction. That is, we won't convert any transaction validation code to Go as part of this BEP.

The Tendermint team already maintains a framework/starting point framework for writing an "ABCI application" (like BigchainDB) in Go, so we could use that code as a starting point, i.e. we can just import it.

@ldmberman
Copy link
Contributor Author

Basically, I am trying to assemble the particular issues this BEP will try to solve.

So far I can clearly see two of them: the performance and the ease of maintenance.

As for the performance, do we have a particular bottleneck? Or do we want to sqeeze the maximum out of a particular metric (like a transaction rate) so that we try to measure it while implementing the BEP? Another problem is, the approach A (the new HTTP API endpoint), while being a reasonable temporary solution, is likely to compromise the performance a little and thus spoil our possible measurements.

Also, I have doubts about the maintenance effort. It seems straightforward to do the initial integration to talk to the new HTTP endpoint, indeed. But the further development might come at a bigger cost compared to the Python path, even considering what will come after we rewrite what we have.

@ttmc
Copy link
Contributor

ttmc commented May 8, 2018

In my opinion, this BEP is mostly about replacing py-abci (maintained by one guy and occasional volunteers) with a Go equivalent that's maintained by the actual Tendermint team (and therefore up-to-date).

We don't want to spend time maintaining or developing code that's not central to what BigchainDB is. In this case, it's the ABCI interface code, which is a Tendermint-specific thing. Let Tendermint take care of that.

@ldmberman
Copy link
Contributor Author

Can you post a link to what you are referring to as the Go equivalent?

I've found some simple examples (including an example of a Python ABCI server by the way) there. We can use one of them but we still need to implement a custom startup script and the HTTP communication - an HTTP server instead of an ABCI server is not something they seem to offer.

I understand the maintenance benefit of turning the ABCI server into the HTTP server. It is straightforward to implement, only requires a little bit of Go, and hopefully (!) does not hurt the performance too much. If this is what we want to do, then it's a simple and independent task, not the strangler approach to the application rewrite.

@ttmc
Copy link
Contributor

ttmc commented May 8, 2018

The Tendermint Ecosystem page in the Tendermint docs lists several ABCI Server implementations, including the Go one under the tendermint/abci repo. That one seems a bit old. I think the reason is that they've been focused on a Go-based Tendermint application development toolkit called "Cosmos-SDK" which includes the ABCI Server plus a whole pile of other useful stuff. We don't have to use the stake stuff; I gather that that's an optional plugin.

@kansi
Copy link
Contributor

kansi commented May 8, 2018

@ldmberman Following are some responses to your concerns

We might want to investigate how to replace Tendermint's storage backend. It feels big and probably deserves a separate BEP. It might involve

As you mentioned its a huge task and deserves its own BEP. Our initial investigation revealed that the Tendermint's backend logic is consolidated and there is a golang MongoDB module which could be used for reference.

Approaching A is likely worse performance-wise compared to the tcp connections created in advance for ABCI. Do you consider Approach B to be more complicated even if we rewrite ABCI method by ABCI method keeping the not yet migrated ABCI calls go through py-abci?

Couldn't understand the concern clearly but neither of those approaches (A and B) made it to the final design. A hybrid approach which takes some parts of A and B has been adopted.

What prevents us from consolidating this logic independently from this BEP?

The concern of this BEP is not re-factoring but rather moving to an officially supported ABCI library.

What are the exact issues we face here? If we have only 2 processes (mongo and bigchaindb-tendermint), would it make it better?

The major pain point is the interation between ABCI proxy app and Tendermint. Some design decisions by Tendermint team have made this communication channel less fault tolerant. It would always be better option for us to choose an implemtation that can avoid using this communication channel.

Do we have some rough ideas about how to unify the configuration? Should it be a different PEP?

I don't think there is a easy and clean of merging BigchainDB and Tendermint's configuration. In my humble opinion it will lead to more mess.

@ttmc
Copy link
Contributor

ttmc commented May 9, 2018

The idea of merging the BigchainDB and Tendermint configuration settings (or not) can also be deferred to another BEP. One of the reasons for BEPs is to force us to think clearly and completely about the options, and the pros and cons of each. (They have other purposes too, such as being open and transparent, giving coders something to implement, giving testers something to test, and giving documentation-writers a good starting point.)

@ldmberman
Copy link
Contributor Author

The concern of this BEP is not re-factoring but rather moving to an officially supported ABCI library.

There does not seem to be such a thing as officially supported ABCI library. Does not prevent us from getting rid of the ABCI socket connections though.

So far I take it as the goal of the BEP is to get rid of the ABCI socket connections. I think, we should rename the BEP to reflect it and clarify its motivation a little. I can come up with a PR.

@ttmc ttmc added the discuss existing BEP Use this issue like a forum for discussion a specific BEP that already exists label May 9, 2018
@ttmc
Copy link
Contributor

ttmc commented May 9, 2018

There does not seem to be such a thing as officially supported ABCI library.

For Go, I think the officially supported library/SDK is Cosmos-SDK.

For JavaScript, I think it's LotionJS.

@kansi
Copy link
Contributor

kansi commented May 9, 2018

There does not seem to be such a thing as officially supported ABCI library.

I would consider the go-abci library developed by Tendermint folks to be the official library.

PS: IIRC go-abci is also included in Tendermint's code base.

@ldmberman
Copy link
Contributor Author

go-abci is one of their core libraries that is used to some extent no matter what the ABCI integration is, yes.

I do not exactly see yet how Cosmos-SDK can help us achieve the goal.

My point is, our problem is not the library, otherwise we would just use the Python example from the go-abci repo, but the ABCI socket connections.

@kansi
Copy link
Contributor

kansi commented May 9, 2018

My point is, our problem is not the library, otherwise we would just use the Python example from the go-abci repo, but the ABCI socket connections.

Correct me if I am wrong but there are no tests for the python example code which you are refering to. I also don't see clearly how production ready that python code is. To me it seems that we can't just use that Python example code and expect BigchainDB to be stable. We would have to write substantial tests to make it production ready, ensure that the Python code is kept up to date with latest Tendermint's wire format etc. So, IMHO the library is the problem to quite an extent.

@ldmberman
Copy link
Contributor Author

I agree, using that example would likely bring issues to the table - I mentioned it because it is a part of an "official library".

To implement the BEP, we need to write some custom Go code based solely on the Tendermint itself as a library - a custom startup script and an Application implementation, I do not see how any other libraries can help so far.

@kansi
Copy link
Contributor

kansi commented May 11, 2018

@vrde @ttmc @ldmberman One performace issue I see with the current adopted approach is follows,

  • During begin_block -> deliver_tx -> end_block cycle we maintian a list of already received transactions .
  • In deliver_tx, the new transaction is validated against the blockchain and the previously received transactions in this block.
  • Since we plan to call /validate api for this we would need to send all the previously received transactions in this block.

To me it seems that there will be a lot of repeated data flow because each block will have hundereds of transactions.

@ldmberman
Copy link
Contributor Author

ldmberman commented May 15, 2018

To sum up the offline discussion we had with @kansi @vrde @ttmc @z-bowen @muawiakh @shahbazn @codegeschrei:

One of the main goals of the BigchainDB platform is performance. We aim to get as big of the transaction rate as possible.

We anticipate the rewrite of BigchainDB in Go to bring significant performance gains on various fronts.

BEP-10 offers a reasonable first step towards the goal but since there is a huge maintenance tradeoff in it we want to properly benchmark BigchainDB to gain enough confidence in making this tradeoff.

Additionally, we want to isolate certain pieces of the platform, prototype alternative implementations of those pieces in Go, and compare them.

Specifically, we want to try the following (note that some of the bullet points are independent):

  • compare sample in-process and remote Python ABCI server implementations with each other;
  • get a performance flamegraph of the Python BigchainDB process under heavy load and identify the most badly behaving parts;
  • think if it makes sense to write up certain functionality in Golang to estimate the future performance gain.

@ttmc ttmc closed this as completed Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss existing BEP Use this issue like a forum for discussion a specific BEP that already exists
Projects
None yet
Development

No branches or pull requests

3 participants