fluent-bit icon indicating copy to clipboard operation
fluent-bit copied to clipboard

plugins: fix compilation if FLB_SQLDB (sqlite3) is disabled

Open ThomasDevoogdt opened this issue 8 months ago β€’ 32 comments

Fixes:

  • https://github.com/fluent/fluent-bit/issues/9757.

Summary by CodeRabbit

  • New Features

    • SQLDB support is now optional; enabling it unlocks partial-match checklist and enhanced blob upload tracking.
    • Added a new post-upload action: "Add Suffix".
  • Bug Fixes

    • Non-database builds now clearly reject DB-only modes/events and fail fast when unsupported.
    • Improved init and cleanup paths to prevent leftover state on failure.
  • Tests

    • CI and tests run both DB-enabled and DB-disabled variants; DB-only tests are excluded when DB is off.

✏️ Tip: You can customize this high-level summary in your review settings.

ThomasDevoogdt avatar Apr 22 '25 20:04 ThomasDevoogdt

Let's make sure this compiles for all existing targets as well.

Would it be better to disable the features within the plugins that need the DB rather than the whole plugin @leonardo-albertovich ? It feels a bit like a large hammer, e.g. no tail input even if you don't use db. We would have to make the config options and any other usage conditional though so it may be worse.

patrick-stephens avatar Apr 24 '25 08:04 patrick-stephens

Let's make sure this compiles for all existing targets as well.

How can it not? Currently, FLB_SQLDB is enabled by default, and will stay like that. The problem is the other way around, if not enabled, then compilation breaks.

Would it be better to disable the features within the plugins that need the DB rather than the whole plugin @leonardo-albertovich ? It feels a bit like a large hammer, e.g. no tail input even if you don't use db. We would have to make the config options and any other usage conditional though so it may be worse.

For me fine, but I would do that on a per feature basis. Perhaps just continue with this PR, and then fine-tune some features that might compile with some small fixups.

In general, it would be much better if all options are toggled in the automated tests, on one reference compilation system. Perhaps even by incremental builds. But either way, I just want to fix compilation now.

ThomasDevoogdt avatar Apr 24 '25 20:04 ThomasDevoogdt

@patrick-stephens @edsiper I changed this PR so that plugins are still compiled, but without database support. I hope this answers https://github.com/fluent/fluent-bit/pull/10239#issuecomment-2826839994.

Tested by doing this:

cd build/
cmake -GNinja -DFLB_SQLDB=OFF ../
ninja

and

cd build/
cmake -GNinja -DFLB_PREFER_SYSTEM_LIBS=ON -DFLB_SQLDB=OFF ../
ninja

ThomasDevoogdt avatar May 02 '25 11:05 ThomasDevoogdt

Not sure if the CI failure is relevant or something else

patrick-stephens avatar May 02 '25 18:05 patrick-stephens

Not sure if the CI failure is relevant or something else

I just added #ifdef FLB_HAVE_SQLDB, which is true by default, so I don't think it's relevant.

ThomasDevoogdt avatar May 02 '25 22:05 ThomasDevoogdt

@edsiper All merge checks seems to be passing. Can you consider this PR? This will also close #9757.

ThomasDevoogdt avatar May 08 '25 17:05 ThomasDevoogdt

In the case of FLB_EVENT_TYPE_BLOBS this constant should not be removed if don't have database support.

I can do that, but where exactly should I warn about it? Should I add one in flb_input_blob_file_register?

In the case of filter_checklist we should issue an error if mode is set to partial and FLB_HAVE_SQLDB is not defined, we cannot accept the patch as is because it would allow fluent-bit to build but cause a silent failure which would be way worse.

Is it fine to add a warning in init_config in plugins/filter_checklist/checklist.c.

In the case of in_blob since it cannot operate without a database it should be disabled in the build script.

But why do I see then #ifdef FLB_HAVE_SQLDB checks in the in_blob plugin if it should be disabled anyway? My commit just fixed one missing #ifdef FLB_HAVE_SQLDB which was forgotten.

See: https://github.com/search?q=repo%3Afluent%2Ffluent-bit%20path%3A%2F%5Eplugins%5C%2Fin_blob%5C%2F%2F%20FLB_HAVE_SQLDB&type=code.

ThomasDevoogdt avatar May 24 '25 19:05 ThomasDevoogdt

@leonardo-albertovich Can you answer my questions? I'm starting to consider to just make a PR which makes sqlite3 a hard requirement. Since no one is really caring to support fluent-bit without it. @edsiper any considerations?

ThomasDevoogdt avatar Jun 27 '25 09:06 ThomasDevoogdt

I'd probably be interested in an in-memory only option as well to remove more dependencies for hardening in those places where we need that and just ephemeral usage.

patrick-stephens avatar Jun 30 '25 09:06 patrick-stephens

I'd probably be interested in an in-memory only option as well to remove more dependencies for hardening in those places where we need that and just ephemeral usage.

I don't really follow here. Please create a relevant patch set yourself. I'm only interested in fixing various compilation issues for integration with Buildroot.

ThomasDevoogdt avatar Jun 30 '25 10:06 ThomasDevoogdt

Walkthrough

Gates SQLDB usage behind FLB_HAVE_SQLDB across core and multiple plugins: conditional includes, guarded DB init/insert/check/close, no-backend stubs in blob DB, updated file-part iterator signature, guarded tests and CI matrix entries; partial-match mode now requires SQLDB or init fails.

Changes

Cohort / File(s) Summary
Checklist plugin SQLDB gating
plugins/filter_checklist/checklist.c, plugins/filter_checklist/checklist.h
Wraps SQLDB includes, DB members and helpers with #ifdef FLB_HAVE_SQLDB; CHECK_PARTIAL_MATCH now conditional; partial-match mode errors if SQLDB disabled; guards db_insert/db_check; adds init failure cleanup.
In-blob plugin guards & constant
plugins/in_blob/blob.c, plugins/in_blob/blob.h
Makes flb_sqldb.h include and blob_db_close() call conditional on FLB_HAVE_SQLDB; adds POST_UPLOAD_ACTION_ADD_SUFFIX = 3 macro.
Azure Blob plugin SQLDB isolation
plugins/out_azure_blob/azure_blob.c, plugins/out_azure_blob/azure_blob.h, plugins/out_azure_blob/azure_blob_conf.c
Moves SQLDB includes and DB helpers behind FLB_HAVE_SQLDB; gates delete/register/timer functions and BLOBS event handling; non-SQLDB builds return errors for BLOBS paths; DB open/close and related mutex guarded.
Azure BlockBlob include only
plugins/out_azure_blob/azure_blob_blockblob.c
Adds #include <fluent-bit/flb_utils.h>; no behavioral change.
Blob DB no-backend stubs & API updates
src/flb_blob_db.c
Adds no-backend stub implementations returning NO_BACKEND_AVAILABLE when SQLDB unset; introduces flb_blob_db_lock/flb_blob_db_unlock; adds remote-id update stubs; updates flb_blob_db_file_part_get_next signature to include int *part_count.
CI matrix
.github/workflows/unit-tests.yaml
Adds -DFLB_SQLDB=On and -DFLB_SQLDB=Off entries to unit-tests matrix.
Tests & runtime shell
tests/runtime/filter_checklist.c, tests/runtime_shell/CMakeLists.txt
Guards mode_partial test with FLB_HAVE_SQLDB; includes in_tail_expect.sh in shell unit tests only when FLB_SQLDB is enabled.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant User as User/Config
  participant Checklist as filter_checklist
  participant DB as SQLDB (optional)

  rect rgba(230,240,255,0.5)
  Note over User,Checklist: Initialization
  User->>Checklist: init_config(mode)
  alt FLB_HAVE_SQLDB and mode==partial
    Checklist->>DB: db_init()
  else
    Checklist->>Checklist: exact-match path or fail init for partial
  end
  end

  rect rgba(235,255,235,0.5)
  Note over Checklist: Loading patterns
  Checklist->>Checklist: load_file_patterns()
  alt partial && FLB_HAVE_SQLDB
    Checklist->>DB: db_insert(pattern)
  else
    Checklist->>Checklist: insert into hash table
  end
  end

  rect rgba(255,245,230,0.5)
  Note over Checklist: Filtering
  Checklist->>Checklist: cb_checklist_filter(record)
  alt partial && FLB_HAVE_SQLDB
    Checklist->>DB: db_check(record)
  else
    Checklist->>Checklist: hash lookup
  end
  end
sequenceDiagram
  autonumber
  participant In as in_blob
  participant Out as out_azure_blob
  participant DB as SQLDB (optional)
  participant Timer as UploadTimer (optional)

  Note over In: Exit handling
  alt FLB_HAVE_SQLDB
    In->>DB: blob_db_close()
  else
    In->>In: skip DB close
  end

  Note over Out: Worker init
  alt FLB_HAVE_SQLDB
    Out->>Timer: azb_timer_create()
  else
    Out->>Out: BLOBS unsupported (error)
  end

  Note over Out: send_blob / flush
  Out->>Out: send_blob(event)
  alt event==BLOBS && FLB_HAVE_SQLDB
    Out->>DB: register/delete parts, manage uploads
  else event==BLOBS && !FLB_HAVE_SQLDB
    Out-->>Out: return FLB_ERROR (BLOBS unsupported)
  else
    Out-->>Out: normal non-BLOBS path
  end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Pay extra attention to:
    • src/flb_blob_db.c β€” exported API changes (flb_blob_db_file_part_get_next) and new no-backend stubs.
    • plugins/filter_checklist/* β€” guarded partial-mode logic and init failure cleanup.
    • plugins/out_azure_blob/* β€” BLOBS gating, timer lifecycle, and error-return paths.
    • CI/test guards β€” ensure conditional test inclusion matches CI matrix.

Possibly related PRs

  • fluent/fluent-bit#10919 β€” touches blob DB cleanup/exit paths used by in_blob; related to guarding DB close code.
  • fluent/fluent-bit#10784 β€” modifies src/flb_blob_db.c and SQLDB wrappers; related to API/wrapper changes here.

Suggested reviewers

  • edsiper
  • leonardo-albertovich
  • patrick-stephens
  • koleini

Poem

"I hopped through macros, quick and spry,
I fenced the DB where partials lie.
Timers hum only when SQL is found,
Exact matches keep the common ground.
A nibble, a patch β€” code carrots found!" πŸ‡βœ¨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.56% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
βœ… Passed checks (2 passed)
Check name Status Explanation
Description Check βœ… Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check βœ… Passed The title accurately summarizes the main objective: adding conditional FLB_SQLDB guards to enable compilation when sqlite3 is disabled.
✨ Finishing touches
  • [ ] πŸ“ Generate docstrings
πŸ§ͺ Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

πŸ“œ Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 808154482f1b07cd586e20069f0608acb83b1670 and 37aeb405afd23148e2e5a63b8557dc2610f7f020.

πŸ“’ Files selected for processing (12)
  • .github/workflows/unit-tests.yaml (1 hunks)
  • plugins/filter_checklist/checklist.c (8 hunks)
  • plugins/filter_checklist/checklist.h (2 hunks)
  • plugins/in_blob/blob.c (2 hunks)
  • plugins/in_blob/blob.h (1 hunks)
  • plugins/out_azure_blob/azure_blob.c (9 hunks)
  • plugins/out_azure_blob/azure_blob.h (1 hunks)
  • plugins/out_azure_blob/azure_blob_blockblob.c (1 hunks)
  • plugins/out_azure_blob/azure_blob_conf.c (4 hunks)
  • src/flb_blob_db.c (5 hunks)
  • tests/runtime/filter_checklist.c (2 hunks)
  • tests/runtime_shell/CMakeLists.txt (1 hunks)
βœ… Files skipped from review due to trivial changes (1)
  • plugins/out_azure_blob/azure_blob.h
🚧 Files skipped from review as they are similar to previous changes (9)
  • plugins/in_blob/blob.c
  • tests/runtime_shell/CMakeLists.txt
  • tests/runtime/filter_checklist.c
  • plugins/out_azure_blob/azure_blob_blockblob.c
  • .github/workflows/unit-tests.yaml
  • plugins/out_azure_blob/azure_blob.c
  • plugins/filter_checklist/checklist.h
  • plugins/out_azure_blob/azure_blob_conf.c
  • plugins/in_blob/blob.h
🧰 Additional context used
🧠 Learnings (1)
πŸ“š Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.

Applied to files:

  • src/flb_blob_db.c
🧬 Code graph analysis (1)
plugins/filter_checklist/checklist.c (3)
src/flb_filter.c (1)
  • flb_filter_set_context (737-740)
include/fluent-bit/flb_mem.h (1)
  • flb_free (126-128)
src/flb_sqldb.c (1)
  • flb_sqldb_close (129-149)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (85)
  • GitHub Check: PR - container builds / Windows container images (2025)
  • GitHub Check: PR - container builds / Windows container images (2022)
  • GitHub Check: PR - packages build MacOS / call-build-macos-package (Intel macOS runner, macos-14-large, 3.31.6)
  • GitHub Check: PR - packages build MacOS / call-build-macos-package (Apple Silicon macOS runner, macos-14, 3.31.6)
  • GitHub Check: PR - packages build Linux / ubuntu/24.04.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / ubuntu/24.04 package build and stage to S3
  • GitHub Check: PR - packages build Linux / centos/8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / ubuntu/22.04 package build and stage to S3
  • GitHub Check: PR - packages build Linux / raspbian/bookworm package build and stage to S3
  • GitHub Check: PR - packages build Linux / debian/bullseye package build and stage to S3
  • GitHub Check: PR - packages build Linux / ubuntu/22.04.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / debian/trixie package build and stage to S3
  • GitHub Check: PR - packages build Linux / debian/trixie.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / debian/buster package build and stage to S3
  • GitHub Check: PR - packages build Linux / debian/bullseye.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / debian/buster.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / debian/bookworm.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / debian/bookworm package build and stage to S3
  • GitHub Check: PR - packages build Linux / almalinux/10.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / almalinux/9 package build and stage to S3
  • GitHub Check: PR - packages build Linux / rockylinux/10 package build and stage to S3
  • GitHub Check: PR - packages build Linux / almalinux/10 package build and stage to S3
  • GitHub Check: PR - packages build Linux / almalinux/8.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / almalinux/9.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / rockylinux/10.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / rockylinux/9.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / rockylinux/8.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / almalinux/8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / centos/9 package build and stage to S3
  • GitHub Check: PR - packages build Linux / rockylinux/9 package build and stage to S3
  • GitHub Check: PR - packages build Linux / centos/7.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / rockylinux/8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / centos/8.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / centos/10.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / centos/10 package build and stage to S3
  • GitHub Check: PR - packages build Linux / amazonlinux/2 package build and stage to S3
  • GitHub Check: PR - packages build Linux / centos/9.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / amazonlinux/2023.arm64v8 package build and stage to S3
  • GitHub Check: PR - packages build Linux / centos/7 package build and stage to S3
  • GitHub Check: PR - packages build Linux / amazonlinux/2023 package build and stage to S3
  • GitHub Check: PR - packages build Linux / amazonlinux/2.arm64v8 package build and stage to S3
  • GitHub Check: PR - container builds / arm/v7/production container image build
  • GitHub Check: PR - container builds / arm/v7/debug container image build
  • GitHub Check: PR - container builds / arm64/debug container image build
  • GitHub Check: PR - container builds / amd64/debug container image build
  • GitHub Check: PR - container builds / amd64/production container image build
  • GitHub Check: PR - container builds / arm64/production container image build
  • GitHub Check: PR - packages build Windows / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
  • GitHub Check: PR - packages build Windows / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
  • GitHub Check: PR - packages build Windows / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
  • GitHub Check: PR - fuzzing test
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SQLDB=Off, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SQLDB=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SQLDB=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SQLDB=Off, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
  • GitHub Check: pr-compile-centos-7
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
  • GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
  • GitHub Check: pr-compile-without-cxx (3.31.6)
πŸ”‡ Additional comments (12)
plugins/filter_checklist/checklist.c (7)

30-127: LGTM! SQLDB helper functions are properly guarded.

The static helper functions db_init, db_insert, and db_check are correctly wrapped with #ifdef FLB_HAVE_SQLDB guards, ensuring they compile only when SQLDB support is enabled.


179-183: LGTM! Database insert properly guarded.

The db_insert call for CHECK_PARTIAL_MATCH mode is correctly wrapped with #ifdef FLB_HAVE_SQLDB, ensuring it's only invoked when SQLDB support is available.


216-222: LGTM! Fail-fast behavior correctly implemented.

The code now properly fails during initialization when mode=partial is requested without SQLDB support, preventing runtime failures. The error message clearly indicates the build-time requirement.


235-242: LGTM! Database initialization properly guarded.

The db_init call is correctly wrapped with #ifdef FLB_HAVE_SQLDB and includes proper error handling.


298-302: LGTM! Error propagation correctly implemented.

The error handling now properly checks the return value from init_config and performs complete cleanup (clearing context, freeing memory) before propagating the error. This prevents the NULL pointer dereference issue identified in previous reviews.


523-527: LGTM! Database check properly guarded.

The db_check call is correctly wrapped with #ifdef FLB_HAVE_SQLDB. Combined with the fail-fast initialization logic, this ensures PARTIAL_MATCH operations never execute without SQLDB support.


613-619: LGTM! Cleanup code properly guarded.

The database cleanup code (finalizing statements and closing the database) is correctly wrapped with #ifdef FLB_HAVE_SQLDB, ensuring it only executes when SQLDB support is enabled.

src/flb_blob_db.c (5)

20-24: LGTM! Header includes properly guarded.

The flb_sqldb.h header is correctly wrapped with #ifdef FLB_HAVE_SQLDB, preventing build failures when SQLDB support is disabled, while keeping flb_blob_db.h unconditionally included.


1415-1423: LGTM! Lock/unlock stubs properly implemented.

The no-backend stubs for flb_blob_db_lock and flb_blob_db_unlock consistently return FLB_BLOB_DB_ERROR_NO_BACKEND_AVAILABLE, matching the pattern of other stubs in this file.


1469-1474: LGTM! Remote ID update stub properly implemented.

The no-backend stub for flb_blob_file_update_remote_id has a signature matching the SQLDB implementation and consistently returns FLB_BLOB_DB_ERROR_NO_BACKEND_AVAILABLE.


1488-1493: LGTM! File part remote ID update stub properly implemented.

The no-backend stub for flb_blob_db_file_part_update_remote_id has a signature matching the SQLDB implementation and consistently returns FLB_BLOB_DB_ERROR_NO_BACKEND_AVAILABLE.


1540-1541: LGTM! Signature correctly synchronized.

The no-backend stub for flb_blob_db_file_part_get_next has been updated to include the new int *part_count parameter, maintaining signature consistency with the SQLDB implementation (lines 957-969).


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.

❀️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Sep 10 '25 08:09 coderabbitai[bot]

@leonardo-albertovich some of your remarks were fixed, can you have a look again?

ThomasDevoogdt avatar Oct 04 '25 11:10 ThomasDevoogdt

@leonardo-albertovich I've updated this PR. Some new errors were fixed since. Can you have a look and eventually merge this?

ThomasDevoogdt avatar Oct 26 '25 06:10 ThomasDevoogdt

@cosmo0920 Can you review this PR? Failing tests should be fixed once https://github.com/fluent/fluent-bit/pull/11077 gets merged.

ThomasDevoogdt avatar Oct 28 '25 20:10 ThomasDevoogdt

This PR seems good but we're not able to add πŸ‘ because of failing unit testing.

Branch is rebased, remaining coderabbitai remarks are fixed. Can you merge it now? +cc @edsiper

ThomasDevoogdt avatar Nov 03 '25 09:11 ThomasDevoogdt

I think in_blob, flb_blob_db must be disabled if sqlite is not available, since they must have the state.

Out S3 and out_azure will need to be aware of the lack of flb_blob_db so they can disable blob handling support

edsiper avatar Nov 04 '25 13:11 edsiper

@edsiper

I think in_blob, flb_blob_db must be disabled if sqlite is not available, since they must have the state.

But why do I then see a bunch of other FLB_HAVE_SQLDB declaratives in flb_blob_db?

e.g. https://github.com/fluent/fluent-bit/blob/ed06c31991fb0c5e3f59677ceb63cabbf50ed1a7/plugins/in_blob/blob.c#L641

If it has to be disabled, do you expect me to drop all existing FLB_HAVE_SQLDB declaratives? Because I somehow expect that the original code should just compile, but was somehow broken over time. What I try to fix now.

ThomasDevoogdt avatar Nov 04 '25 15:11 ThomasDevoogdt

in_blob doesn't need to be disabled because it can operate without a database flb_blob_db does't need to be excluded because when FLB_HAVE_SQLDB isn't detected it impleemnts the exact same functions except they all return FLB_BLOB_DB_ERROR_NO_BACKEND_AVAILABLE as the error code out_azure_blob doesn't need to be disabled because when FLB_HAVE_SQLDB isn't detected it just doesn't process blobs out_s3 doesn't need to be disabled because when FLB_HAVE_SQLDB isn't detected it just doesn't process blobs

That's of course as far as I know and as a side note, the test case issue is unrelated to this feature because it's flb-rt-core_chunk_trace.

leonardo-albertovich avatar Nov 04 '25 16:11 leonardo-albertovich

in_blob doesn't need to be disabled because it can operate without a database flb_blob_db does't need to be excluded because when FLB_HAVE_SQLDB isn't detected it impleemnts the exact same functions except they all return FLB_BLOB_DB_ERROR_NO_BACKEND_AVAILABLE as the error code out_azure_blob doesn't need to be disabled because when FLB_HAVE_SQLDB isn't detected it just doesn't process blobs out_s3 doesn't need to be disabled because when FLB_HAVE_SQLDB isn't detected it just doesn't process blobs

That's of course as far as I know and as a side note, the test case issue is unrelated to this feature because it's flb-rt-core_chunk_trace.

Then I don't touch anything. I would appreciate that this can just be merged then. Thanks for the review!

ThomasDevoogdt avatar Nov 04 '25 19:11 ThomasDevoogdt

Oh no, I didn't review the PR, I just clarified how things worked originally and what the CI issue was.

leonardo-albertovich avatar Nov 04 '25 21:11 leonardo-albertovich

moving this until the next milestone, some parts are still moving

edsiper avatar Nov 09 '25 21:11 edsiper

@edsiper 4.2 has been released, can you merge it now?

ThomasDevoogdt avatar Nov 14 '25 18:11 ThomasDevoogdt

@edsiper @leonardo-albertovich @cosmo0920 @patrick-stephens, you probably forgot about this one. Since v4.2 has been released, it's perhaps time to merge this one.

ThomasDevoogdt avatar Nov 19 '25 21:11 ThomasDevoogdt

This looks good to me. However, this review comment seems not to be addressed: https://github.com/fluent/fluent-bit/pull/10239/files#r2492077239 Is it enough with this implementation?

I fixed/added some warnings for when FLB_SQLDB is not compiled. I would think its good to go now?

ThomasDevoogdt avatar Nov 20 '25 17:11 ThomasDevoogdt

We fixed no left device error on our CI. So, could you rebase off current master? We need to confirm all of the CI tasks are passed before processing review.

cosmo0920 avatar Nov 21 '25 05:11 cosmo0920

We fixed no left device error on our CI. So, could you rebase off current master? We need to confirm all of the CI tasks are passed before processing review.

Rebased, and all tests seems to be fine. Can we merge?

ThomasDevoogdt avatar Nov 21 '25 08:11 ThomasDevoogdt

@leonardo-albertovich just checking your comments were covered?

patrick-stephens avatar Nov 21 '25 12:11 patrick-stephens

I'll review this PR again tomorrow. @ThomasDevoogdt if you have any questions that were unanswered (I think I missed some) please repeat them so I can address them tomorrow.

leonardo-albertovich avatar Nov 21 '25 19:11 leonardo-albertovich

I'll review this PR again tomorrow. @ThomasDevoogdt if you have any questions that were unanswered (I think I missed some) please repeat them so I can address them tomorrow.

I don't have any open questions. If you see any shortcomings in this PR, then I will try to address them. But beware that I won't do any in depth refactorings, for that, someone closser to the matter should step in.

ThomasDevoogdt avatar Nov 21 '25 21:11 ThomasDevoogdt

I'll review this PR again tomorrow. @ThomasDevoogdt if you have any questions that were unanswered (I think I missed some) please repeat them so I can address them tomorrow.

@leonardo-albertovich I think this PR is finalized, it would be nice having it merged. By this, the runtime tests would also start to work, limiting the chance that I have to rework this anytime soon.

ThomasDevoogdt avatar Nov 25 '25 19:11 ThomasDevoogdt