Skip to content

Conversation

@Cheun99
Copy link
Contributor

@Cheun99 Cheun99 commented Sep 1, 2024

Purpose of this pull request

new feature #7510

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

@liunaijie
Copy link
Member

Please enable your repo's action

@liunaijie liunaijie added the First-time contributor First-time contributor label Sep 2, 2024

@Override
public CompletableFuture<Void> releaseWorker(String workerID) {
public CompletableFuture<Void> releaseWorker(String workerId) {
Copy link
Member

Choose a reason for hiding this comment

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

please do not update this as not use in this pr


@Override
public CompletableFuture<Void> releaseWorker(String workerID) {
public CompletableFuture<Void> releaseWorker(String workerId) {
Copy link
Member

Choose a reason for hiding this comment

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

ditto


List<SlotProfile> getAssignedSlots(Map<String, String> tags);

void registerWorker(Address address,WorkerProfile workerProfile);
Copy link
Member

Choose a reason for hiding this comment

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

why need add this new method?

NodeEngineImpl nodeEngine = seaTunnelServer.getNodeEngine();
MemberImpl localMember = nodeEngine.getLocalMember();
if (!params.isEmpty() && params.containsKey("tags")) {
localMember.updateAttribute((Map<String, String>) params.get("tags"));
Copy link
Member

Choose a reason for hiding this comment

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

if this tags value is not a Map<String, String>, what's the response?

and if not tags is null or empty, is this will clear the node tags?

@Cheun99 Cheun99 marked this pull request as ready for review September 3, 2024 09:10
}

@Test
public void testUpdateTags() {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a test case for update tag failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@liunaijie
Copy link
Member

LGTM if ci pass

Copy link
Member

@Hisoka-X Hisoka-X left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @Cheun99

@Hisoka-X Hisoka-X merged commit d1c75f7 into apache:dev Sep 6, 2024
@Hisoka-X
Copy link
Member

Hisoka-X commented Sep 7, 2024

Hi @Cheun99 , how about add a link into

to tell user we can update node tags when use resource isolation feature?

@Cheun99
Copy link
Contributor Author

Cheun99 commented Sep 9, 2024

Hi @Cheun99 , how about add a link into

to tell user we can update node tags when use resource isolation feature?

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants