Skip to content

Gb 7247 remove now useless response metadata simplify operation#9

Open
hackal wants to merge 4 commits intomainfrom
gb-7247-remove-now-useless-response-metadata-simplify-operation
Open

Gb 7247 remove now useless response metadata simplify operation#9
hackal wants to merge 4 commits intomainfrom
gb-7247-remove-now-useless-response-metadata-simplify-operation

Conversation

@hackal
Copy link
Contributor

@hackal hackal commented Aug 13, 2024

Description by Cal

PR Description

This PR refactors the codebase by removing the now redundant response metadata and simplifying the operation cache handling. It replaces the HotCache with OperationCache, updates related interfaces, and removes unused code. Additionally, it introduces configuration files for automated PR reviews using Callstack.ai.

Key Issues

None

Files Changed

File: /.callstack.yml Added configuration for Callstack.ai PR review automation.
File: /.github/workflows/callstack-reviewer.yml Added GitHub Actions workflow for Callstack.ai PR review.
File: /cli/crates/federated-dev/src/dev/gateway_nanny.rs Updated cache factory type to `OperationCacheFactory`.
File: /engine/crates/engine-v2/src/engine.rs Refactored to use `OperationCache` instead of `HotCache` and removed response metadata.
File: /engine/crates/engine-v2/src/engine/cache.rs Removed unused cache key types and refactored document handling.
File: /engine/crates/engine-v2/src/engine/runtime.rs Updated cache factory type to `OperationCacheFactory`.
File: /engine/crates/engine-v2/src/engine/trusted_documents.rs Refactored document handling and removed redundant code.
File: /engine/crates/engine-v2/src/http_response/mod.rs Removed `HttpGraphqlResponseExtraMetadata` and related logic.
File: /engine/crates/integration-tests/src/federation/builder/bench.rs Updated cache factory type to `InMemoryOperationCacheFactory`.
File: /engine/crates/integration-tests/src/federation/builder/test_runtime.rs Updated cache factory type to `InMemoryOperationCacheFactory`.
File: /engine/crates/runtime-local/src/lib.rs Removed `hot_cache` module and added `operation_cache` module.
File: /engine/crates/runtime-local/src/operation_cache.rs Introduced `InMemoryOperationCache` and `InMemoryOperationCacheFactory`.
File: /engine/crates/runtime/src/lib.rs Removed `hot_cache` module and added `operation_cache` module.
File: /engine/crates/runtime/src/operation_cache.rs Refactored to use `OperationCache` instead of `HotCache`.
File: /gateway/crates/federated-server/src/server/gateway.rs Updated cache factory type to `OperationCacheFactory`.
# Description

Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.

Type of change

  • 💔 Breaking
  • 🚀 Feature
  • 🐛 Fix
  • 🛠️ Tooling
  • 🔨 Refactoring
  • 🧪 Test
  • 📦 Dependency
  • 📖 Requires documentation update

Finistere and others added 4 commits August 6, 2024 15:22
…hing

Now that operation checks rely on ClickHouse, no need for response
metadata anymore. Also removed the trusted document cache, the operation
cache already fills that role and having two in-memory caches means
twice the monitoring.
Copy link

@callstackai-action callstackai-action bot left a comment

Choose a reason for hiding this comment

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

PR Review Summary

This pull request has been reviewed. Please check the comments and suggestions provided.

&self.hooks
}
fn cache_factory(&self) -> &Self::CacheFactory {
fn operation_cache_factory(&self) -> &Self::OperationCacheFactory {

Choose a reason for hiding this comment

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

Suggestion
The operation_cache_factory method currently returns a reference to an empty tuple &(). This might be a placeholder or a temporary implementation. If this is intended to be a no-op or a default implementation, consider adding a comment to clarify its purpose. Otherwise, ensure that this implementation aligns with the intended functionality of the OperationCacheFactory.

}

impl OperationCacheFactory for InMemoryOperationCacheFactory {
type Cache<V> = InMemoryOperationCache<V>

Choose a reason for hiding this comment

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

🐛 Bug
There is a missing semicolon at the end of the type definition for Cache<V> in the OperationCacheFactory implementation. In Rust, type definitions require a semicolon at the end.

Suggested change
type Cache<V> = InMemoryOperationCache<V>
type Cache<V> = InMemoryOperationCache<V>;

Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong.

self.inner.insert(key, value);
}

async fn get(&self, key: &String) -> Option<V> {

Choose a reason for hiding this comment

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

🐛 Bug
The get method in the OperationCache implementation for InMemoryOperationCache<V> is missing an await keyword. Since self.inner.get(key) is an asynchronous operation, it should be awaited to ensure the operation completes before returning the result.

Suggested change
async fn get(&self, key: &String) -> Option<V> {
self.inner.get(key).await

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.

3 participants