Skip to content

repair: refactor to remove callbacks #4886

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

Closed

Conversation

emwang-jump
Copy link
Contributor

@emwang-jump emwang-jump commented Apr 25, 2025

pushed directly to repair-replay-link

@emwang-jump emwang-jump changed the base branch from main to chali/feat/repair-replay-link April 25, 2025 21:31
@emwang-jump emwang-jump force-pushed the emwang/repair-clean branch 4 times, most recently from 1a0b692 to 2a832c3 Compare April 28, 2025 14:58
@emwang-jump emwang-jump changed the title Emwang/repair clean repair: refactor to remove callbacks Apr 28, 2025
@emwang-jump emwang-jump force-pushed the emwang/repair-clean branch from 2a832c3 to 5c56d94 Compare April 28, 2025 15:04
Copy link
Member

@lidatong lidatong left a comment

Choose a reason for hiding this comment

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

generally my idea here is to make fd_repair more library vs. "framework-y" by filling values that are returned to the caller

benefits include makes it easier to unit test, reduces nesting, separates concerns in the function

FD_TEST(0 == fd_repair_protocol_encode(&protocol, &ctx));
ulong buflen = (ulong)((uchar*)ctx.data - buf);

repair_send_intake_packet( buf, buflen, peer_addr, self_ip4_addr, repair_tile_ctx );
Copy link
Member

Choose a reason for hiding this comment

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

what i actually was thinking is this would just fill the packet inside a caller provided buf arg

fd_repair_handle_ping( fd_repair_tile_ctx_t *  repair_tile_ctx,
                       fd_repair_t *                 glob,
                       fd_gossip_ping_t const *      ping,
                       fd_gossip_peer_addr_t const * peer_addr,
                       uint                          self_ip4_addr,
                       uchar * buf,
                       ulong * buf_sz ) {

and then the caller would be responsible for calling repair_send_intake_packet (which can probably be deleted and just directly call send_packet)

as opposed to invoking the function itself

if( shred == NULL ) {
FD_LOG_WARNING(("invalid shread"));
} else {
repair_shred_deliver(shred, shredlen, src_addr, &val->id, repair_tile_ctx);
Copy link
Member

Choose a reason for hiding this comment

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

similar to above, i think a cleaner API would be to not actually call repair_shred_deliver

(also we can probably clean this up. the mutation of active table should be managed by caller separately vs. inside this function. but this can happen in a later PR)

fd_memcpy( buf + 4U, &sig, 64U );

uint src_ip4_addr = 0U; /* unknown */
repair_send_intake_packet( buf, buflen, addr, src_ip4_addr, repair_tile_ctx );
Copy link
Member

Choose a reason for hiding this comment

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

same comment as above

FD_TEST(0 == fd_repair_response_encode(&gmsg, &ctx));
ulong buflen = (ulong)((uchar*)ctx.data - buf);

repair_send_serve_packet( buf, buflen, dst_addr, src_ip4_addr, repair_tile_ctx );
Copy link
Member

Choose a reason for hiding this comment

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

same

}
fd_hash_copy( &val->id, &header->sender );
val->good = 0;
fd_repair_send_ping( repair_tile_ctx, glob, peer_addr, self_ip4_addr, val );
Copy link
Member

Choose a reason for hiding this comment

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

same, but dont worry about changing this function since we don't need it right now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants