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

merge indexer #3609

Merged

Conversation

zhangsoledad
Copy link
Member

@zhangsoledad zhangsoledad commented Sep 23, 2022

What problem does this PR solve?

Early on, CKB had built-in indexer-related functionality, but because of implementation issues and driven by the micro-kernel design philosophy at the time, we decided to make the indexer independent.
As more and more developers gave us feedback about the convenience, the indexer service was an extra burden for development convenience and maintenance, so we rethought the relationship between indexer and ckb-core, and decided to merge indexer into ckb-core, so that developers can start the built-in indexer simply by command-line arguments or configuration

ckb run -C <path>  --indexer

or

[rpc]
modules = ["...", "Indexer"]

instead of downloading a binary and running a separate service.

Technically speaking, the built-in indexer also has some advantages. With rocksdb's support for second instances, the built-in indexer can directly synchronize data as a secondary_db instead of rpc, building indexes at a speed of about 39%.

Some notes

  • bcb51b1 The reason this refactoring is included in this PR is that the original notify uses crossbeam's blocking channel, and the indexer currently spawn into the tokio runtime, so if the indexer needs to subscribe to the transaction pool, it needs to use notify, but the blocking channel is not well handled in tokio's environment, and the spawn_blocking like tokio provides is not very applicable, spawn_blocking feature is intended for blocking operations that eventually finish, the async channel provided by Tokio can call outside of asynchronous contexts, but in turn the blocking channel is not really suitable for async, so here by the way the notify is completely replaced by async channel.

  • util/indexer is based on ckb-indexer v0.4.1, some types used for serialization in util/indexer/src/service.rs should be merged into ckb-json-type, and the PR after this one may do the related refactoring.

  • To distinguish it from the original legacy configuration, the indexer configuration now has v2 as a suffix

[indexer_v2]
# Indexing the pending txs in the ckb tx-pool
index_tx_pool = false

Check List

Tests

  • Unit test
  • Integration test

Release note

Title Only: Include only the PR title in the release note.

@zhangsoledad zhangsoledad requested a review from a team as a code owner September 23, 2022 09:29
@zhangsoledad zhangsoledad requested review from doitian and removed request for a team September 23, 2022 09:29
@zhangsoledad zhangsoledad force-pushed the zhangsoledad/merge-indexer-customize branch 4 times, most recently from 880710d to 34618c8 Compare September 28, 2022 09:32
@zhangsoledad zhangsoledad changed the title indexer customize rule merge indexer Sep 28, 2022
@zhangsoledad zhangsoledad mentioned this pull request Sep 28, 2022
@zhangsoledad
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Oct 8, 2022
3609:  merge indexer r=zhangsoledad a=zhangsoledad

<!--
Thank you for contributing to nervosnetwork/ckb!

If you haven't already, please read [CONTRIBUTING](https://github.com/nervosnetwork/ckb/blob/develop/CONTRIBUTING.md) document.

If you're unsure about anything, just ask; somebody should be along to answer within a day or two.

PR Title Format:
1. module [, module2, module3]: what's changed
2. *: what's changed
-->

### What problem does this PR solve?

Early on, CKB had built-in indexer-related functionality, but because of implementation issues and driven by the micro-kernel design philosophy at the time, we decided to make the indexer independent.
As more and more developers gave us feedback about the convenience, the indexer service was an extra burden for development convenience and maintenance, so we rethought the relationship between indexer and ckb-core, and decided to merge indexer into ckb-core, so that developers can start the built-in indexer simply by command-line arguments or configuration
```bash
ckb run -C <path>  --indexer
```
or 
``` toml
[rpc]
modules = ["...", "Indexer"]
```
instead of downloading a binary and running a separate service.

Technically speaking, the built-in indexer also has some advantages. With rocksdb's support for second instances, the built-in indexer can directly synchronize data as a secondary_db instead of rpc, building indexes at a speed of about 39%.

### Some notes
* bcb51b1 The reason this refactoring is included in this PR is that the original notify uses crossbeam's blocking channel, and the indexer currently spawn into the tokio runtime, so if the indexer needs to subscribe to the transaction pool, it needs to use notify, but the blocking channel is not well handled in tokio's environment, and the spawn_blocking like tokio provides is not very applicable, spawn_blocking feature is intended for blocking operations that eventually finish, the async channel provided by Tokio can call outside of asynchronous contexts, but in turn the blocking channel is not really suitable for async, so here by the way the notify is completely replaced by async channel. 

* `util/indexer` is based on [ckb-indexer](https://github.com/nervosnetwork/ckb-indexer) v0.4.1, some types used for serialization in util/indexer/src/service.rs should be merged into ckb-json-type, and the PR after this one may do the related refactoring.

* To distinguish it from the original legacy configuration, the indexer configuration now has v2 as a suffix
```toml
[indexer_v2]
# Indexing the pending txs in the ckb tx-pool
index_tx_pool = false
```

* The changes to the Rpc interface compared to the original [ckb-indexer](https://github.com/nervosnetwork/ckb-indexer) are:
           *[get_tip](https://github.com/nervosnetwork/ckb-indexer#get_tip) was renamed to `get_indexer_tip`, to avoid ambiguity
           *[get_indexer_info](https://github.com/nervosnetwork/ckb-indexer#get_indexer_info) removed

### Check List <!--REMOVE the items that are not applicable-->

Tests <!-- At least one of them must be included. -->

- Unit test
- Integration test


### Release note <!-- Choose from None, Title Only and Note. Bugfixes or new features need a release note. -->

```release-note
Title Only: Include only the PR title in the release note.
```



Co-authored-by: zhangsoledad <787953403@qq.com>
@bors
Copy link
Contributor

bors bot commented Oct 8, 2022

Build failed:

  • ci_unit_tests_ubuntu

bors bot added a commit that referenced this pull request Oct 8, 2022
3609:  merge indexer r=zhangsoledad a=zhangsoledad

<!--
Thank you for contributing to nervosnetwork/ckb!

If you haven't already, please read [CONTRIBUTING](https://github.com/nervosnetwork/ckb/blob/develop/CONTRIBUTING.md) document.

If you're unsure about anything, just ask; somebody should be along to answer within a day or two.

PR Title Format:
1. module [, module2, module3]: what's changed
2. *: what's changed
-->

### What problem does this PR solve?

Early on, CKB had built-in indexer-related functionality, but because of implementation issues and driven by the micro-kernel design philosophy at the time, we decided to make the indexer independent.
As more and more developers gave us feedback about the convenience, the indexer service was an extra burden for development convenience and maintenance, so we rethought the relationship between indexer and ckb-core, and decided to merge indexer into ckb-core, so that developers can start the built-in indexer simply by command-line arguments or configuration
```bash
ckb run -C <path>  --indexer
```
or 
``` toml
[rpc]
modules = ["...", "Indexer"]
```
instead of downloading a binary and running a separate service.

Technically speaking, the built-in indexer also has some advantages. With rocksdb's support for second instances, the built-in indexer can directly synchronize data as a secondary_db instead of rpc, building indexes at a speed of about 39%.

### Some notes
* bcb51b1 The reason this refactoring is included in this PR is that the original notify uses crossbeam's blocking channel, and the indexer currently spawn into the tokio runtime, so if the indexer needs to subscribe to the transaction pool, it needs to use notify, but the blocking channel is not well handled in tokio's environment, and the spawn_blocking like tokio provides is not very applicable, spawn_blocking feature is intended for blocking operations that eventually finish, the async channel provided by Tokio can call outside of asynchronous contexts, but in turn the blocking channel is not really suitable for async, so here by the way the notify is completely replaced by async channel. 

* `util/indexer` is based on [ckb-indexer](https://github.com/nervosnetwork/ckb-indexer) v0.4.1, some types used for serialization in util/indexer/src/service.rs should be merged into ckb-json-type, and the PR after this one may do the related refactoring.

* To distinguish it from the original legacy configuration, the indexer configuration now has v2 as a suffix
```toml
[indexer_v2]
# Indexing the pending txs in the ckb tx-pool
index_tx_pool = false
```

* The changes to the Rpc interface compared to the original [ckb-indexer](https://github.com/nervosnetwork/ckb-indexer) are:
           *[get_tip](https://github.com/nervosnetwork/ckb-indexer#get_tip) was renamed to `get_indexer_tip`, to avoid ambiguity
           *[get_indexer_info](https://github.com/nervosnetwork/ckb-indexer#get_indexer_info) removed

### Check List <!--REMOVE the items that are not applicable-->

Tests <!-- At least one of them must be included. -->

- Unit test
- Integration test


### Release note <!-- Choose from None, Title Only and Note. Bugfixes or new features need a release note. -->

```release-note
Title Only: Include only the PR title in the release note.
```



Co-authored-by: zhangsoledad <787953403@qq.com>
@nervosnetwork nervosnetwork deleted a comment from bors bot Oct 8, 2022
@zhangsoledad zhangsoledad force-pushed the zhangsoledad/merge-indexer-customize branch from 89da2b0 to ace2bb5 Compare October 8, 2022 05:00
@zhangsoledad
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 8, 2022

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

Successfully merging this pull request may close these issues.

None yet

2 participants