MC: Add CP subsystem mgmt operations to client #16226
Conversation
puzpuzpuz
force-pushed
the
v4.0/enhancement/mc-client-cp-subsystem-mgmt
branch
from
6e0d8fa
to
ce43c90
Compare
|
run-lab-run |
puzpuzpuz
requested review from
emre-aydin
and removed request for
emre-aydin
| List<SimpleEntry<String, String>> result = new ArrayList<>(); | ||
| for (CPMember cpMember : cpMembers) { | ||
| String address = "[" + cpMember.getAddress().getHost() + "]:" + cpMember.getAddress().getPort(); | ||
| result.add(new SimpleEntry<>(cpMember.getUuid().toString(), address)); |
There was a problem hiding this comment.
I have commented also in protocol pr. We should use uuid type in the protocol and avoid uuid to string conversion.
There was a problem hiding this comment.
The type was changed.
@sancar could you take another look at this PR tomorrow?
|
|
||
| @Override | ||
| public String getMethodName() { | ||
| return "mcGetCPMembers"; |
There was a problem hiding this comment.
Needs to be getCPMembers. We use the method name in the protocol definition. Same for other message tasks.
There was a problem hiding this comment.
My concern is potential collision with public client operations with the same name (getCPMembers) that could be introduced in future. Do you think I should add mc prefixes into client protocol for these methods, or it's fine to get rid of them here?
There was a problem hiding this comment.
That I don't know much about. Someone from the client team probably knows more. @asimarslan @sancar @ihsandemir what do you think?
There was a problem hiding this comment.
We are trying to match the method name to method name that user calls. If there is no User API, then the correct choice is not really well defined. In that sense, putting mc prefix to avoid a future collision is a good choice.
Even if we are trying to match the names to protocol, I am not aware of it. @asimarslan What do you think ?
There was a problem hiding this comment.
As the correct approach is not certain, I've removed prefixes from #getMethodNames.
puzpuzpuz
force-pushed
the
v4.0/enhancement/mc-client-cp-subsystem-mgmt
branch
from
21b7c3f
to
02021f0
Compare
| nodeEngine.getHazelcastInstance().getCPSubsystem().getCPSubsystemManagementService(); | ||
| return cpService.getCPMembers().toCompletableFuture() | ||
| .thenApply(cpMembers -> { | ||
| List<SimpleEntry<String, String>> result = new ArrayList<>(); |
There was a problem hiding this comment.
Why do we have List<Entry<String, String>> here instead of CPMemberDTO?
There was a problem hiding this comment.
That's because we have to deal with data types defined for codecs. And the general guideline in the client protocol generator is to avoid adding custom types where possible. That's why List<CPMember> is represented as List<Map.Entry<String, String>>.
A side note: it's going to change from List<Map.Entry<String, String>> to List<Map.Entry<UUID, Address>> (see comment from @sancar).
puzpuzpuz
force-pushed
the
v4.0/enhancement/mc-client-cp-subsystem-mgmt
branch
from
a0671d6
to
c0bed93
Compare
|
run-lab-run |
1 similar comment
|
run-lab-run |
|
Merging this PR as the only failed test is a known issue and has nothing to do with local changes: |
getCPMemberspromoteToCPMemberremoveCPMemberresetCPSubsystemProtocol changes: hazelcast/hazelcast-client-protocol#276