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.

Sign up for GitHub

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

Jump to bottom

Add StatsManager in graphd #1301

Merged
merged 9 commits into from
Nov 29, 2019
Merged

Add StatsManager in graphd #1301

merged 9 commits into from
Nov 29, 2019

Conversation

laura-ding
Copy link
Contributor

@laura-ding laura-ding commented Nov 21, 2019 •
edited
Loading

close #1272

@laura-ding laura-ding added the ready-for-testing PR: ready for the CI test label Nov 21, 2019
@jude-zhu
Copy link
Contributor

close #1272

@jude-zhu jude-zhu requested a review from CPWstatic November 21, 2019 06:11
@whitewum
Copy link
Contributor

@Amber1990Zhang I think we need a doc

@nebula-community-bot
Copy link
Member

Unit testing passed.

@nebula-community-bot
Copy link
Member

Unit testing failed.

@critical27 critical27 mentioned this pull request Nov 22, 2019
Merged
@nebula-community-bot
Copy link
Member

Unit testing passed.

1 similar comment
@nebula-community-bot
Copy link
Member

Unit testing passed.

critical27
critical27 previously approved these changes Nov 26, 2019
View reviewed changes
Copy link
Contributor

@critical27 critical27 left a comment

Choose a reason for hiding this comment

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

LGTM

@nebula-community-bot
Copy link
Member

Unit testing passed.

yixinglu
yixinglu reviewed Nov 28, 2019
View reviewed changes
@nebula-community-bot
Copy link
Member

Unit testing passed.

yixinglu
yixinglu previously approved these changes Nov 29, 2019
View reviewed changes
Copy link
Contributor

@yixinglu yixinglu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I suggest that a new base executor class with stats_, for example named StatsBaseExecutor, should be provided and other subclass type could inherit it when they need some statistics.

class StatsBaseExecutor : public Executor {
public:
  StatsBaseExecutor(const char *name) {
    stats_ = StatsManager::getStats(name);
  }
  onError(xx) {
    onError(xx, stats_)
  }
  onFinish() {
    onFinish(stats_);
  }
protected:
  std::unique_ptr<Stats> stats_;
};

Later, we can refactor this implementation.

@nebula-community-bot
Copy link
Member

Unit testing passed.

1 similar comment
@nebula-community-bot
Copy link
Member

Unit testing passed.

@nebula-community-bot
Copy link
Member

Unit testing passed.

@dangleptr dangleptr merged commit d79ed19 into vesoft-inc:master Nov 29, 2019
@jude-zhu
Copy link
Contributor

relate to #185

yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Feb 16, 2020
@laura-ding @dangleptr
Add StatsManager in graphd (vesoft-inc#1301)
87c5790
tong-hao pushed a commit to tong-hao/nebula that referenced this pull request Jun 1, 2021
@laura-ding @dangleptr
Add StatsManager in graphd (vesoft-inc#1301)
5251bc9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shylock-Hg Shylock-Hg Shylock-Hg left review comments

@yixinglu yixinglu yixinglu approved these changes

@critical27 critical27 critical27 approved these changes

@dutor dutor Awaiting requested review from dutor

@CPWstatic CPWstatic Awaiting requested review from CPWstatic

@darionyaphet darionyaphet Awaiting requested review from darionyaphet

Assignees
No one assigned
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add StatsManager in graphd
9 participants
@laura-ding @jude-zhu @whitewum @nebula-community-bot @yixinglu @darionyaphet @critical27 @Shylock-Hg @dangleptr