Skip to content
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

refactor: remove postgres #1064

Closed
wants to merge 12 commits into from
Closed

Conversation

mayconamaroCW
Copy link
Contributor

@mayconamaroCW mayconamaroCW commented Jun 11, 2024

remove postgres storage, CSV importer/exporter and ignore bytecode tests

Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]

4

🧪 Relevant tests

No

🔒 Security concerns

No

⚡ Key issues to review

Background Operations Logging:
The PR removes extensive logging and tracing capabilities which were previously implemented. This could impact the ability to monitor and debug the system effectively. It's recommended to ensure that sufficient logging is maintained.

Async and Blocking Tasks:
The PR removes the CSV exporter and associated asynchronous tasks. Ensure that the remaining tasks are correctly categorized as async or blocking according to the system's architecture requirements.

Removal of PostgreSQL Support:
The PR completely removes support for PostgreSQL storage. This is a significant change and should be thoroughly reviewed to ensure that it aligns with the strategic goals and does not affect users who might be relying on this feature.

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Reliability
Add error handling for thread spawning to enhance application robustness

Implement error handling for the thread spawning process to ensure that the application
can gracefully handle failures during thread creation, which is critical for maintaining
robustness and reliability.

src/bin/importer_offline.rs [72]

-let storage_thread = thread::Builder::new().name("storage-loader".into());
+let storage_thread = thread::Builder::new().name("storage-loader".into())
+    .spawn(|| {
+        // Thread logic here
+    })
+    .expect("Failed to spawn storage-loader thread");
 
Suggestion importance[1-10]: 9

Why: Implementing error handling for thread spawning is essential for ensuring that the application can gracefully handle failures, thereby improving robustness and reliability.

9
Enhancement
Replace the CSV export functionality with an alternative storage mechanism

Replace the removed CSV export functionality with a new storage mechanism or ensure that
the current permanent storage system is capable of handling the data previously managed by
the CSV system. This change is crucial to maintain data integrity and system functionality
after the removal of the CSV-related code.

src/bin/importer_offline.rs [72]

 let storage_thread = thread::Builder::new().name("storage-loader".into());
+// Initialize new storage mechanism here
 
Suggestion importance[1-10]: 8

Why: This suggestion addresses a significant change in the PR by proposing an alternative to the removed CSV export functionality, which is crucial for maintaining data integrity and system functionality.

8
Maintainability
Refactor block handling into a separate function to improve readability and maintainability

Refactor the execute_block_importer function to encapsulate the logic for handling blocks
and receipts into separate functions. This will improve code readability and
maintainability by breaking down complex functions into smaller, more manageable parts.

src/bin/importer_offline.rs [156-159]

 for block in blocks.into_iter() {
+    handle_block(block)?;
+}
+
+fn handle_block(block: ExternalBlock) -> anyhow::Result<()> {
     if GlobalState::warn_if_shutdown(TASK_NAME) {
         return Ok(());
     }
+    // Additional block handling logic here
+    Ok(())
+}
 
Suggestion importance[1-10]: 8

Why: Refactoring the execute_block_importer function to encapsulate block handling logic into separate functions enhances code readability and maintainability by breaking down complex functions into smaller, more manageable parts.

8
Introduce a feature toggle for alternative export formats

Since the CSV functionality has been removed, consider adding a feature toggle or
configuration option that allows users to enable or disable alternative export formats,
providing flexibility in how data is exported and stored.

src/bin/importer_offline.rs [72]

 let storage_thread = thread::Builder::new().name("storage-loader".into());
+// Add feature toggle for alternative export formats here
 
Suggestion importance[1-10]: 7

Why: Adding a feature toggle for alternative export formats provides flexibility and maintainability, allowing users to choose how data is exported and stored.

7

@mayconamaroCW
Copy link
Contributor Author

Removing the tests was enough to speed up CI a little. I'll reopen this PR later.

@mayconamaroCW mayconamaroCW deleted the refactor/remove-postgres branch June 13, 2024 13:06
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.

1 participant