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
[FLINK-8087] Decouple Slot from AllocatedSlot #5088
Conversation
84d86be
to
e3f9150
Compare
This commit introduces the SlotContext which is an abstraction for the SimpleSlot to obtain the relevant slot information to do the communication with the TaskManager without relying on the AllocatedSlot which is now only used by the SlotPool. This closes apache#5088.
e3f9150
to
dd139a4
Compare
This commit introduces the SlotContext which is an abstraction for the SimpleSlot to obtain the relevant slot information to do the communication with the TaskManager without relying on the AllocatedSlot which is now only used by the SlotPool. This closes apache#5088.
This commit introduces the SlotContext which is an abstraction for the SimpleSlot to obtain the relevant slot information to do the communication with the TaskManager without relying on the AllocatedSlot which is now only used by the SlotPool. This closes apache#5088.
@@ -31,7 +30,7 @@ | |||
import static org.mockito.Mockito.mock; | |||
import static org.mockito.Mockito.when; | |||
|
|||
public class AllocatedSlotsTest { | |||
public class SlotsTestImplContext { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this renamed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a mistake. Will correct it.
* <p> | ||
* This method should be removed once the new interface-based RPC abstraction is in place | ||
* | ||
* @return The actor gateway that can be used to send messages to the TaskManager. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that TaskManagerGateway
can be an actor gateway is not something that is relevant for the reader of the Javadoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove it.
@@ -312,24 +312,36 @@ public void returnAllocatedSlot(Slot slot) { | |||
// (1) do we have a slot available already? | |||
SlotAndLocality slotFromPool = availableSlots.poll(resources, locationPreferences); | |||
if (slotFromPool != null) { | |||
SimpleSlot slot = createSimpleSlot(slotFromPool.slot(), slotFromPool.locality()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method createSimpleSlot
is no longer in use and can be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. Will do it.
return true; | ||
} | ||
} else { | ||
throw new RuntimeException("Unsupported logical slot type encounterd " + logicalSlot.getClass()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: encounterd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Will correct it.
The cluster entrypoints start the ResourceManager with the web interface URL. This URL is used to set the correct tracking URL in Yarn when registering the Yarn application. This closes apache#5128.
This commit introduces the SlotContext which is an abstraction for the SimpleSlot to obtain the relevant slot information to do the communication with the TaskManager without relying on the AllocatedSlot which is now only used by the SlotPool. This closes apache#5088.
dd139a4
to
0945d9e
Compare
Thanks for the review @GJL. Merging this PR. |
What is the purpose of the change
This commit introduces the SlotContext which is an abstraction for the SimpleSlot
to obtain the relevant slot information to do the communication with the
TaskManager without relying on the AllocatedSlot which is now only used by the
SlotPool.
This PR is based on #5087.
Brief change log
SlotContext
as simple abstraction for slot related informationSlot
onAllocatedSlot
which is now only used internally by theSlotPool
.SimpleSlotContext
which implementsSlotContext
and acts as the slot context for theSimpleSlot
.Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation
CC: @GJL