-
Notifications
You must be signed in to change notification settings - Fork 60
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
Block till sufficient space is available #57
Conversation
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.
Looks good as far as I can tell. There is one minor point that I left in a comment, but it's not urgent and you should probably merge this as-is.
skylark/gateway/gateway_reciever.py
Outdated
@@ -92,6 +92,12 @@ def recv_chunks(self, conn: socket.socket, addr: Tuple[str, int]): | |||
# receive header and write data to file | |||
chunk_header = WireProtocolHeader.from_socket(conn) | |||
logger.debug(f"[server:{server_port}] Got chunk header {chunk_header.chunk_id}: {chunk_header}") | |||
|
|||
# wait for free space (at least space for two chunks) | |||
while self.chunk_store.remaining_bytes() < chunk_header.chunk_len * 2: |
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.
Something to keep in mind is that, even if you are waiting in this while loop, data received from the source gateway (or previous gateway) is still being buffered by TCP, and takes up memory in the form of the TCP receive buffer in the kernel. If this becomes a problem, we might want to have a protocol for this gateway to send a message to the previous gateway indicating that it is ready, and for the previous gateway to receive this message before starting the transfer. You wait for two chunks' worth of space already, but you have 16 connections per gateway that is sending you data, so it might be worth bumping up that constant to take this into account.
The code you have is fine for now, but if you still get out of memory problems down the road, then this might be the issue (though I'm fine waiting until this actually becomes a problem before fixing this).
commit a6ad91b Merge: 5106140 e616614 Author: Paras Jain <[email protected]> Date: Sun Jan 16 00:33:20 2022 -0800 Merge branch 'main' into dev/paras/block_2.0 commit 5106140 Author: Paras Jain <[email protected]> Date: Sun Jan 16 05:10:18 2022 +0000 Fixed version of block for free space (fixing #57)
commit a6ad91b Merge: 5106140 e616614 Author: Paras Jain <[email protected]> Date: Sun Jan 16 00:33:20 2022 -0800 Merge branch 'main' into dev/paras/block_2.0 commit 5106140 Author: Paras Jain <[email protected]> Date: Sun Jan 16 05:10:18 2022 +0000 Fixed version of block for free space (fixing #57)
Enables running gateways on instances with less RAM such as Azure.