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

Use listenerId to distinguish multiple add listeners to one space #5032

Conversation

panda-sheep
Copy link
Contributor

@panda-sheep panda-sheep commented Dec 9, 2022

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Description:

  1. Use listenerId to distinguish different add listeners.
    Synchronized from the enterprise version https://github.com/vesoft-inc/nebula-ent/pull/1860.
    solve the problem of adding listener immediately after remove listener.
    close Use listenerId to distinguish multiple add listeners to one space #5027

  2. Modify the code to keep the community version and enterprise version consistent
    and optimize some code to make the code cleaner.

  3. Add UT and improve UT code

How do you solve it?

Because remove listener and add listener are asynchronous operations.
In the past, after removing the listener, you need to wait for a while to add the listener, otherwise an error will occur.
After the current modification, after remove listener, you can add listener immediately.

Test case as follows:

step 1:
create space space1(partition_num=1, replica_factor=1, vid_type = FIXED_STRING(30));
use space1
create tag player(name string, age int);
insert vertex player(name, age) values "11":("aaaa",1);
insert vertex player(name, age) values "22":("aaaa",2);
insert vertex player(name, age) values "33":("aaaa",3);
insert vertex player(name, age) values "44":("aaaa",4);
FETCH PROP ON player "11" yield player.name;
sign in text service(192.168.8.215:47500);
show text search clients;

step2:
add listener ELASTICSEARCH 192.168.8.211:56700;
show listener

step 3:
REMOVE LISTENER ELASTICSEARCH;
show listener
add listener ELASTICSEARCH 192.168.8.211:56700;
show listener

step 4:
The value in the ListenerId file is different, meet our expectations.

The first ListenerId:
:$ ll
18:35 listnenerId
:
/nebula_test_tool$ ./readListenerId xxxx/storage_listener/listener/1/1/wal/listnenerId
listenerId 3

The second ListenerId:
:$ ll
18:38 listnenerId
:
/nebula_test_tool$ ./readListenerId xxxxx/storage_listener/listener/1/1/wal/listnenerId
listenerId 4

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

@panda-sheep panda-sheep requested a review from a team as a code owner December 9, 2022 04:00
@panda-sheep panda-sheep force-pushed the use_listenerId_to_fix_remove_listener_and_add_listener branch from 7ac5b19 to 6fa6783 Compare December 9, 2022 04:36
@panda-sheep panda-sheep added ready-for-testing PR: ready for the CI test ready for review labels Dec 9, 2022
@panda-sheep panda-sheep force-pushed the use_listenerId_to_fix_remove_listener_and_add_listener branch 2 times, most recently from eae158d to 26243ab Compare December 12, 2022 10:21
@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2022

Codecov Report

Base: 77.24% // Head: 77.26% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (71966f8) compared to base (cc0ce34).
Patch coverage: 72.98% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5032      +/-   ##
==========================================
+ Coverage   77.24%   77.26%   +0.02%     
==========================================
  Files        1105     1105              
  Lines       82054    82185     +131     
==========================================
+ Hits        63383    63503     +120     
- Misses      18671    18682      +11     
Impacted Files Coverage Δ
src/clients/meta/MetaClient.h 92.30% <ø> (ø)
src/graph/executor/admin/ShowHostsExecutor.cpp 93.54% <ø> (+0.99%) ⬆️
src/graph/executor/query/ProjectExecutor.cpp 61.11% <ø> (-2.78%) ⬇️
src/graph/optimizer/Optimizer.h 100.00% <ø> (ø)
src/kvstore/NebulaStore.h 78.57% <ø> (ø)
src/kvstore/PartManager.h 71.42% <ø> (-18.05%) ⬇️
src/kvstore/listener/elasticsearch/ESListener.cpp 0.00% <0.00%> (ø)
src/kvstore/listener/elasticsearch/ESListener.h 0.00% <0.00%> (ø)
src/meta/MetaServiceHandler.h 100.00% <ø> (ø)
src/meta/processors/BaseProcessor-inl.h 86.85% <ø> (ø)
... and 65 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@xtcyclist xtcyclist force-pushed the use_listenerId_to_fix_remove_listener_and_add_listener branch from 26243ab to 3d59de6 Compare December 12, 2022 12:56
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.

What will happen when this PR merged:

  1. Does existing listener could start up by NebulaStore::loadLocalListenerFromPartManager?
  2. If 1 works, what is the listener id of existing listener?

I guess we will crash.

return *reinterpret_cast<const PartitionID*>(rawData.data() + offset);
}

ListenerID MetaKeyUtils::parseListenerId(folly::StringPiece rawData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you need to handle the old existing data which does not have listener id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.
Compatibility has already supported.

@critical27
Copy link
Contributor

The listener id's problem (what you discussed that we need remove -> wait -> add), I think we could leave it alone and mark as known issue. My concern is that this PR only works for new cluster but not the existing one.

@panda-sheep panda-sheep force-pushed the use_listenerId_to_fix_remove_listener_and_add_listener branch from 3d59de6 to adf933a Compare December 13, 2022 02:31
@panda-sheep panda-sheep force-pushed the use_listenerId_to_fix_remove_listener_and_add_listener branch from adf933a to 48099f3 Compare December 13, 2022 04:05
@panda-sheep
Copy link
Contributor Author

panda-sheep commented Dec 13, 2022

What will happen when this PR merged:

  1. Does existing listener could start up by NebulaStore::loadLocalListenerFromPartManager?
  2. If 1 works, what is the listener id of existing listener?

I guess we will crash.

Before the upgrade if there is listener, after the upgrade:
1)Existing listener could start up by NebulaStore::loadLocalListenerFromPartManager.
2)use the default listenerId for compatibility.

@panda-sheep panda-sheep force-pushed the use_listenerId_to_fix_remove_listener_and_add_listener branch from 48099f3 to 06b3863 Compare December 13, 2022 05:11
@panda-sheep
Copy link
Contributor Author

guess we will crash.

There is a listener before the upgrade, and it can continue to execute after the upgrade. already tested.@critical27

ListListenerResp listListener(1: ListListenerReq req);
ExecResp addListener(1: AddListenerReq req);
ExecResp removeListener(1: RemoveListenerReq req);
ListListenersResp listListeners(1: ListListenersReq req);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the change of this interface affect the compatibility of different nebula version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.
There is a compatibility effect. I think it can be ignored.
There are two reasons:

  1. This interface is only used by the kernel, not uses by other tools.
  2. The purpose of changing this interface is to keep it consistent with the interface of the enterprise version.

@Sophie-Xie Sophie-Xie closed this Dec 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use listenerId to distinguish multiple add listeners to one space
5 participants