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

merge host info from kvstore #550

Merged
merged 5 commits into from
Jul 2, 2019
Merged

merge host info from kvstore #550

merged 5 commits into from
Jul 2, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jun 26, 2019

fix bug:
#532

spacewalkman
spacewalkman reviewed Jun 26, 2019
View reviewed changes
src/meta/test/ActiveHostsManTest.cpp Outdated
@@ -37,6 +37,56 @@ TEST(ActiveHostsManTest, NormalTest) {
}
}

TEST(ActiveHostsManTest, NormalTest2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should give it more meaningful test name like MergeHostTest ? WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Author

Choose a reason for hiding this comment

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

fixed

dangleptr
dangleptr reviewed Jun 27, 2019
View reviewed changes
Copy link
Contributor

@dangleptr dangleptr left a comment

Choose a reason for hiding this comment

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

The PR looks good now. Please fix the inline comments

src/meta/ActiveHostsMan.cpp Outdated
bool notFound = hostsMap_.find({host.ip, host.port}) == hostsMap_.end();
if (notFound) {
data.emplace_back(MetaServiceUtils::hostKey(host.ip, host.port),
MetaServiceUtils::hostValOffline());
Copy link
Contributor

Choose a reason for hiding this comment

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

Alignment

Copy link
Author

Choose a reason for hiding this comment

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

fixed

src/meta/test/ActiveHostsManTest.cpp Outdated
std::vector<kvstore::KV> data;
for (auto i = 0; i < 3; i++) {
data.emplace_back(MetaServiceUtils::hostKey(0, i),
MetaServiceUtils::hostValOnline());
Copy link
Contributor

Choose a reason for hiding this comment

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

Alignment

Copy link
Author

Choose a reason for hiding this comment

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

fixed

src/meta/test/ActiveHostsManTest.cpp Outdated
@@ -37,6 +37,56 @@ TEST(ActiveHostsManTest, NormalTest) {
}
}

TEST(ActiveHostsManTest, NormalTest2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

src/meta/test/ActiveHostsManTest.cpp Outdated
ASSERT_EQ(0, offlineNum);

ActiveHostsMan ahMan(1, 1, kv.get());
sleep(3);
Copy link
Contributor

Choose a reason for hiding this comment

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

sleep(intervalSeconds + 1)

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@ghost ghost added the ready-for-testing PR: ready for the CI test label Jul 2, 2019
@ghost ghost requested review from dangleptr and critical27 July 2, 2019 02:17
critical27
critical27 approved these changes Jul 2, 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.

Well done!

dangleptr
dangleptr approved these changes Jul 2, 2019
View reviewed changes
Copy link
Contributor

@dangleptr dangleptr 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.

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

Unit testing passed.

@dangleptr dangleptr merged commit a6bbebe into vesoft-inc:master Jul 2, 2019
@critical27 critical27 mentioned this pull request Jul 2, 2019
Closed
dangleptr pushed a commit to dangleptr/nebula that referenced this pull request Jul 5, 2019
merge host info from kvstore (vesoft-inc#550)
1520514 
* merge host info from kvstore

* add read lock

* found to notFound

* align fix
@ghost ghost deleted the merge_host_info branch July 11, 2019 09:33
tong-hao pushed a commit to tong-hao/nebula that referenced this pull request Jun 1, 2021
@dangleptr
merge host info from kvstore (vesoft-inc#550)
e25ab46 
* merge host info from kvstore

* add read lock

* found to notFound

* align fix
critical27 added a commit to critical27/nebula that referenced this pull request Aug 26, 2021
@critical27
cherry-pick vesoft-inc#549 vesoft-inc#550 vesoft-inc#551
27e9f0d
critical27 added a commit to critical27/nebula that referenced this pull request Aug 29, 2021
@critical27
cherry-pick vesoft-inc#549 vesoft-inc#550 vesoft-inc#551
d49d6d6
bright-starry-sky pushed a commit that referenced this pull request Aug 31, 2021
@critical27
Cherry-pick PR from v2.5.0 (#2581)
4dd638e 
* get meta version in hb

* cherry-pick #549 #550 #551

* damn timeout

* fix rebuild index bug introduced in #2557
yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Mar 21, 2022
@nebula-bots @critical27
Cherry-pick PR from v2.5.0 (vesoft-inc#37)
4db27c1 
* get meta version in hb

* cherry-pick vesoft-inc#549 vesoft-inc#550 vesoft-inc#551

* damn timeout

* fix rebuild index bug introduced in vesoft-inc#2557

Co-authored-by: Doodle <[email protected]>
yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Mar 21, 2022
@nebula-bots @SuperYoko
Add ci for standalone compile and ut. (vesoft-inc#550)
da3f3ac 
<!--
Thanks for your contribution!
In order to review PR more efficiently, please add information according to the template.
-->

#### What type of PR is this?
- [x] bug
- [ ] feature
- [ ] enhancement

#### What problem(s) does this PR solve?
Issue(s) number: 

Description:
Add workflow to check standalone compile and ut. PYTEST and TCK comes later.


#### How do you solve it?


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



#### 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:

Please confirm whether to be reflected in release notes and how to describe:
> ex. Fixed the bug .....


Migrated from vesoft-inc#3637

Co-authored-by: Alex Xing <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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.
4 participants
@nebula-community-bot @spacewalkman @critical27 @dangleptr