Refactor Connection Management
- Dominant language
- C
- Stars
- 12.8k
- Forks
- 794
- Avg merge
- 2d 14h
- Merged PRs (30d)
- 31
Description
This is to track improvements we can make to connection management related code, and the issue is to be edited as I get better understanding of what we can improve.
**High-level structural issues**
- [ ] Consolidate the API we use for connection management in different executors. We probably don't need the `connectionId` stuff in `multi_client_executor.c` anymore.
- [ ] The reason behind having the hash tables in `connection_management.c` and `placement_connection.c` are a bit difficult to understand. They also make the code very bloated. It wasn't until I tried to remove `ConnectionHash` that I understood why we exactly need it. I don't have a specific suggestion on how we should make the situation better. I'll experiment with some.
- [ ] `placement_connection.c` is difficult to follow.
**Other structural issues**
- [ ] `FinishConnectionEstablishment()` is similar to `MultiClientConnectPoll()` + `ClientConnectionReady()`. Consolidate them and remove duplicate code.
- [ ] `ShutdownConnection()` seems to be too low level to be called outside `connection_management.c`, check if it is really necessary to be exported.
**Implementation detail issues**
- [ ] `MultiClientSendQuery()` probably should set `connection->remoteTransaction->transactionFailed`, but it doesn't. If this is the case, we should probably call `HandleRemoteTransactionConnectionError()` there. If that is the case, some duplicate code between `multi_router_executor.c` and `multi_client_executor.c`. Extract the common logic into a single function.
- [ ] `GetPlacementConnection()`: In `multi_copy.c` and `multi_explain.c`, we check the status of result of `GetPlacementConnection()` before using it. In `master_stage_protocol.c` and `master_delete_protocol.c` we don't. Should we check it or not? Understand this.
- [ ] Similarly, for `GetNodeConnection()`. Sometimes we check the status of its result, sometimes we don't.
...
(more items for later)
Contributor guide
Assessment
This issue has not been assessed yet.