Stateless sync. Per db tables, indexes, schemas.#3139
Conversation
|
Claude finished @khustup2's task in 1m 59s —— View job PR Review CompleteI've reviewed the stateless sync changes and left inline comments on the issues I found. The PR implements per-database catalog organization for stateless multi-instance sync with schemas and indexes support. Key Changes:
Issues Found:
Memory Management: ✅ Properly handles |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } catch (const std::exception& e) { | ||
| catalog_table_cache::instance().invalidate(); | ||
| elog(ERROR, "Failed to ensure catalog at %s: %s", root_path.c_str(), e.what()); | ||
| elog(ERROR, "Failed to ensure per-db catalog at %s/%s: %s", root_path.c_str(), db_name.c_str(), e.what()); |
There was a problem hiding this comment.
Error handling swallows exceptions and returns 0, but callers may interpret 0 as success. Consider returning -1 or throwing to distinguish errors from empty state.
| elog(ERROR, "Failed to ensure per-db catalog at %s/%s: %s", root_path.c_str(), db_name.c_str(), e.what()); | |
| elog(ERROR, "Failed to ensure per-db catalog at %s/%s: %s", root_path.c_str(), db_name.c_str(), e.what()); | |
| throw; |
| current += c; | ||
| } | ||
| } | ||
| if (!current.empty()) { |
There was a problem hiding this comment.
Trailing comma handling adds empty string to result. If column_names is "col1,col2,", this pushes "col1", "col2", then empty string from remainder after last comma.
| if (!current.empty()) { | |
| if (!current.empty()) { | |
| result.push_back(current); | |
| } | |
| // Don't add empty strings from trailing commas | |
| return result; |
| #include <algorithm> | ||
| #include <cstring> | ||
| #include <unordered_map> | ||
| #include <vector> |
There was a problem hiding this comment.
Sync interval reduced from 2000ms to 500ms (4x more frequent). This increases S3 API calls and costs. Verify this is intentional for production workloads, especially with many databases.
|
1 participant
🚀 🚀 Pull Request
Impact
Description
Things to be aware of
Things to worry about
Additional Context