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
Introduce dynamoDB as a Database implementation #598
Conversation
blockchain/blockchain.go
Outdated
@@ -1090,6 +1095,7 @@ func (bc *BlockChain) writeStateTrie(block *types.Block, state *state.StateDB) e | |||
nodesSize, preimagesSize = trieDB.Size() | |||
nodesSizeLimit = common.StorageSize(bc.cacheConfig.CacheSize) * 1024 * 1024 | |||
) | |||
// TODO-Aidan: increase size limit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO-Aidan: increase size limit | |
// TODO-Klaytn: consider to increase size limit or parameterize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the comment
storage/database/dynamodb.go
Outdated
} | ||
|
||
logger.Info("creating s3FileDB ", "bucket", config.TableName+"-bucket") | ||
s3FileDB, err := newS3FileDB(config.Region, "https://s3.ap-northeast-2.amazonaws.com", config.TableName+"-bucket") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok that url includes ap-northeast-2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as far as I know the region should be specified in the endpoint URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the value will be replaced to the user-configurable value.
storage/database/dynamodb.go
Outdated
} | ||
|
||
logger.Info("creating s3FileDB ", "bucket", config.TableName+"-bucket") | ||
s3FileDB, err := newS3FileDB(config.Region, "https://s3.ap-northeast-2.amazonaws.com", config.TableName+"-bucket") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as far as I know the region should be specified in the endpoint URL.
This PR was a draft which refines some code from https://github.com/winnie-byun/klaytn/tree/dynamo2-noBatch4 |
078fe64
to
e5ec17c
Compare
Reviewers, please take a look. Consider that dynamoDB config values will be configurable later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. :)
Merge this.
@winnie-byun please refine more with some comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
@aidan-kwon For work efficiency, I will refine myself after this PR.
Proposed changes
Types of changes
Please put an x in the boxes related to your change.
Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
$ make test
)