Conversation
thweetkomputer
force-pushed
the
fix-gcs-compile
branch
from
cbe436d to
5fa954e
Compare
WalkthroughAdds preprocessor guards around S3-specific code, moving S3 client factory, AWS includes, Init/Shutdown, credential checks, and endpoint wiring behind Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI/main
participant Core as rocksdb_cloud_dump
participant FS as CloudFS
participant DB as RocksDB
CLI->>Core: start
Core->>Core: prepare cfs_options
alt DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3 defined
Core->>Core: Aws::InitAPI()
Core->>Core: buildS3ClientFactory(endpoint)
Core->>FS: NewAwsFileSystem(cfs_options, s3_factory)
else GCP path
Core->>FS: NewGcpFileSystem(cfs_options)
end
FS->>Core: filesystem ready
Core->>DB: Open DB with filesystem
alt success
DB->>Core: opened
else failure
DB->>Core: error
end
alt S3 initialized
Core->>Core: Aws::ShutdownAPI()
end
Core->>CLI: exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
eloq_data_store_service/rocksdb_cloud_dump.cpp (4)
23-24: Guard AWS headers with conditional compilation.AWS SDK headers are included unconditionally but are only needed when
DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3is defined. This creates unnecessary dependencies when building for GCP.Apply this diff to conditionally include AWS headers:
+#if defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3) #include <aws/core/Aws.h> #include <aws/s3/S3Client.h> +#endif #include <gflags/gflags.h>
142-157: Guard AWS credential validation with conditional compilation.The validation of AWS credentials (lines 142-149) is executed regardless of the backend. When building for GCP, these AWS-specific checks should not be required.
Apply this diff to conditionally validate AWS credentials:
+#if defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3) // Validate required parameters if (params.aws_access_key_id.empty()) { throw std::runtime_error("AWS access key ID is required"); } if (params.aws_secret_key.empty()) { throw std::runtime_error("AWS secret access key is required"); } +#endif if (params.bucket_name.empty()) {
349-376: Critical: Move variable declarations outside the conditional block.Variables
aws_options(line 351),cfs_options(line 355), andstatus(line 369) are declared inside theDATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3guard but are used outside this block:
cfs_optionsis used at lines 378-408aws_optionsis used at lines 468, 521, and 631statusis used at lines 413, 416, and 420When
DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_GCPis defined withoutDATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3, these variables will be undeclared, causing compilation failures. The AI summary mentions that the related filerocksdb_cloud_data_store.cppmoves the status initialization outside the ifdef block, but this pattern is not applied here.Apply this diff to declare variables before the conditional block:
// Create directory for DB path if it doesn't exist std::filesystem::create_directories(params.db_path); + // Declare variables used across multiple backends + rocksdb::Status status; + rocksdb::CloudFileSystemOptions cfs_options; +#if defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3) + Aws::SDKOptions aws_options; +#endif + #if defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3) // Initialize AWS SDK - Aws::SDKOptions aws_options; Aws::InitAPI(aws_options); - // Set up cloud filesystem options - rocksdb::CloudFileSystemOptions cfs_options; - // Set credentials if (!params.aws_access_key_id.empty() && !params.aws_secret_key.empty()) { cfs_options.credentials.InitializeSimple(params.aws_access_key_id, params.aws_secret_key); } else { cfs_options.credentials.type = rocksdb::AwsAccessType::kUndefined; } // Validate credentials - rocksdb::Status status = cfs_options.credentials.HasValid(); + status = cfs_options.credentials.HasValid(); if (!status.ok()) { LOG(ERROR) << "Invalid AWS credentials: " << status.ToString();
468-468: Guard AWS SDK shutdown with conditional compilation.
Aws::ShutdownAPI(aws_options)is called unconditionally, butaws_optionsis only declared whenDATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3is defined. Additionally, lines 521 and 631 have the same issue.Apply this diff at lines 468, 521, and 631:
cloud_env = nullptr; cloud_fs = nullptr; +#if defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3) Aws::ShutdownAPI(aws_options); +#endif google::ShutdownGoogleLogging();
thweetkomputer
force-pushed
the
fix-gcs-compile
branch
from
df8f2e2 to
e353ff1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (2)
22-23: Guard AWS SDK includes with preprocessor directive.These AWS SDK headers are only used in the S3-specific
BuildS3ClientFactorymethod (now guarded at lines 205-266). When building without S3 support, these headers won't be available and will cause compilation errors.Apply this diff to guard the includes:
+#if defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3) #include <aws/core/client/ClientConfiguration.h> #include <aws/s3/S3Client.h> +#endif #include <bthread/condition_variable.h>
205-266: Guard AWS SDK includes in rocksdb_cloud_data_store.cpp to match conditional compilation of S3-specific code.The call site at line 347 is properly guarded with
#ifdef DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3, but the AWS includes at lines 22–23 (#include <aws/core/client/ClientConfiguration.h>and#include <aws/s3/S3Client.h>) are unconditional. This creates a compilation failure whenDATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3is not defined. Wrap these includes with the same preprocessor guard, matching the pattern in rocksdb_cloud_dump.cpp.
🧹 Nitpick comments (1)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
368-372: Add fallback for cloud type in error message.If neither
DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3norDATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_GCSis defined, the error message will have an empty cloud type: "cloud type: , at path...". Consider adding an#elseclause or#errordirective.Apply this diff to improve the error message:
LOG(ERROR) << "Unable to create cloud storage filesystem, cloud type: " #if defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3) << "Aws" #elif defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_GCS) << "Gcp" +#else + << "Unknown" #endif << ", at path rocksdb_cloud with bucket "
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eloq_data_store_service/rocksdb_cloud_data_store.cpp(6 hunks)eloq_data_store_service/rocksdb_cloud_data_store.h(1 hunks)eloq_data_store_service/rocksdb_cloud_dump.cpp(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- eloq_data_store_service/rocksdb_cloud_dump.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
eloq_data_store_service/rocksdb_cloud_data_store.h (1)
eloq_data_store_service/rocksdb_cloud_data_store.cpp (2)
BuildS3ClientFactory(206-265)BuildS3ClientFactory(206-207)
🔇 Additional comments (4)
eloq_data_store_service/rocksdb_cloud_data_store.h (1)
50-58: LGTM! Proper conditional compilation guard.The
BuildS3ClientFactorydeclaration is correctly wrapped in the S3-specific preprocessor guard, matching the implementation in the.cppfile. This ensures the method is only visible when building with S3 support, preventing compile-time and link-time errors.eloq_data_store_service/rocksdb_cloud_data_store.cpp (3)
205-266: LGTM! S3-specific code properly guarded.The
BuildS3ClientFactoryimplementation is correctly wrapped in the S3 conditional compilation guard, ensuring it's only compiled when S3 support is enabled. This matches the corresponding header declaration.
276-299: LGTM! Status variable scope handling is correct.The
statusvariable is properly moved to an outer scope (line 276) and assigned within the S3 guard (line 291), then later assigned fromNewCloudFileSystem(line 364). This pattern ensures the variable is available for both S3-specific credential validation and the common cloud filesystem initialization, with proper control flow in both build configurations.
343-359: LGTM! S3 endpoint configuration properly isolated.The S3-specific endpoint and transfer manager configuration is correctly guarded, ensuring this code only compiles when S3 support is enabled.
Minor style note: This uses
#ifdefwhile line 205 uses#if defined(). Both are equivalent but consider using consistent style throughout.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
eloq_data_store_service/rocksdb_cloud_dump.cpp (1)
144-151: Guard AWS credential validation with S3 conditional compilation.AWS credential validation on lines 144-151 will cause GCP-only builds to fail at runtime, as these parameters won't be relevant for GCP. Wrap this validation block with
#if defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3).Apply this diff:
+#if defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3) // Validate required parameters if (params.aws_access_key_id.empty()) { throw std::runtime_error("AWS access key ID is required"); } if (params.aws_secret_key.empty()) { throw std::runtime_error("AWS secret access key is required"); } +#endif if (params.bucket_name.empty()) {
♻️ Duplicate comments (1)
eloq_data_store_service/rocksdb_cloud_dump.cpp (1)
423-424: LGTM! GCP filesystem creation parameter corrected.The previous issue where
cfswas passed instead of&cfstoNewGcpFileSystemhas been fixed. Both S3 and GCP paths now correctly pass&cfs(pointer-to-pointer).
🧹 Nitpick comments (1)
eloq_data_store_service/rocksdb_cloud_dump.cpp (1)
45-53: Consider guarding AWS-specific flags with conditional compilation.Flags like
aws_access_key_id,aws_secret_key,region, ands3_endpointare AWS-specific. While they don't break compilation in GCP builds, guarding them would improve clarity and prevent user confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eloq_data_store_service/rocksdb_cloud_dump.cpp(9 hunks)
🔇 Additional comments (5)
eloq_data_store_service/rocksdb_cloud_dump.cpp (5)
23-26: LGTM! Correct conditional compilation for AWS includes.The AWS SDK headers are properly guarded to prevent compilation when building for non-S3 cloud storage backends.
170-235: LGTM! S3 client factory properly isolated.The
buildS3ClientFactoryfunction is correctly guarded and will only be compiled for S3 builds, preventing AWS SDK dependencies in GCP builds.
353-355: LGTM! Status variable correctly declared outside guards.The
statusvariable is used in both guarded and unguarded sections (credential validation, filesystem creation, error handling), so declaring it outside the conditional compilation blocks is correct.
408-415: LGTM! S3 endpoint configuration properly guarded.The S3-specific endpoint configuration and client factory assignment are correctly isolated to S3 builds.
475-477: LGTM! AWS cleanup properly guarded across all exit paths.All
Aws::ShutdownAPIcalls are correctly wrapped in S3 conditional compilation guards, matching the initialization at line 359. This ensures AWS SDK is only cleaned up when it was initialized.Also applies to: 530-532, 642-644
thweetkomputer
force-pushed
the
fix-gcs-compile
branch
2 times, most recently
from
bd2585a to
87def1e
Compare
thweetkomputer
force-pushed
the
fix-gcs-compile
branch
from
87def1e to
213b3b7
Compare
2 participants
Summary by CodeRabbit