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

[kvstore] adjust log level and add header comments #3681

Merged
merged 7 commits into from
Feb 9, 2022

Conversation

critical27
Copy link
Contributor

@critical27 critical27 commented Jan 11, 2022

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Description:

  1. Adjust log level and add header comments in src/codec and src/kvstore directory
  2. Remove the HBase directory
  3. Move the implementation of AtomicLogBuffer to source file

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

If any interface's comments is not clear, feel free to ask me to add some description

Checklist:

Tests:

  • Unit test(positive and negative cases)
  • Function test
  • Performance test
  • N/A

Affects:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatibility (If it breaks the compatibility, please describe it and add the label.)
  • If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

No

Copy link
Contributor

@panda-sheep panda-sheep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job

src/codec/RowReaderV1.cpp Outdated Show resolved Hide resolved
src/kvstore/KVEngine.h Outdated Show resolved Hide resolved
src/kvstore/KVEngine.h Outdated Show resolved Hide resolved
src/kvstore/KVEngine.h Show resolved Hide resolved
@@ -107,7 +107,7 @@ bool RowReaderWrapper::reset(meta::SchemaProviderIf const* schema,
currReader_ = &readerV2_;
return true;
} else {
LOG(ERROR) << "Unsupported row reader version " << readerVer;
LOG(WARNING) << "Unsupported row reader version " << readerVer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to distinguish between warning and error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on whether administrator need to be involved. ERROR definitely need to be checked, but WARNING is not mandatory.

Copy link
Contributor

@wenhaocs wenhaocs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work!

src/common/utils/NebulaKeyUtils.cpp Show resolved Hide resolved
src/kvstore/plugins/elasticsearch/ESListener.cpp Outdated Show resolved Hide resolved
src/kvstore/raftex/test/TestShard.cpp Show resolved Hide resolved
src/kvstore/KVStore.h Outdated Show resolved Hide resolved
src/kvstore/KVStore.h Outdated Show resolved Hide resolved
src/kvstore/KVStore.h Outdated Show resolved Hide resolved
src/kvstore/Listener.cpp Show resolved Hide resolved
panda-sheep
panda-sheep previously approved these changes Jan 25, 2022
Copy link
Contributor

@panda-sheep panda-sheep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impressive work!

wenhaocs
wenhaocs previously approved these changes Jan 28, 2022
class RaftLogIterator final : public LogIterator {
public:
/**
* @brief Construct a new raf log iterator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

miss a 't'

@Shinji-IkariG Shinji-IkariG merged commit ce21b2d into vesoft-inc:master Feb 9, 2022
@critical27 critical27 deleted the log branch February 9, 2022 04:58
liwenhui-soul pushed a commit to liwenhui-soul/nebula that referenced this pull request Feb 15, 2022
* adjust log level

* add header comments

* address comments

* address @wenhaocs's comments

* rebased, code format

confirm with critical27
yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Mar 21, 2022
…esoft-inc#598)

* adjust log level

* add header comments

* address comments

* address @wenhaocs's comments

* rebased, code format

confirm with critical27

Co-authored-by: Doodle <13706157+critical27@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants