Skip to content

chore: add debug log for Nacos config snapshot hits#4333

Open
EvanYao826 wants to merge 1 commit into
alibaba:2025.1.xfrom
EvanYao826:fix/nacos-config-data-loader-snapshot
Open

chore: add debug log for Nacos config snapshot hits#4333
EvanYao826 wants to merge 1 commit into
alibaba:2025.1.xfrom
EvanYao826:fix/nacos-config-data-loader-snapshot

Conversation

@EvanYao826

@EvanYao826 EvanYao826 commented May 21, 2026

Copy link
Copy Markdown

What does this PR do?

Adds a debug-level log message when a Nacos config value is loaded from the local snapshot cache instead of being fetched from the remote server.

This aids troubleshooting in cluster mode where snapshot cache hits indicate a config change notification arrived before the remote node synced.

Note: The core stale-config fix was already handled by #4319. This PR only adds observability for snapshot cache behavior.

@CLAassistant

CLAassistant commented May 21, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@EvanYao826

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

@EvanYao826 EvanYao826 force-pushed the fix/nacos-config-data-loader-snapshot branch from d352f17 to 868ea6a Compare May 21, 2026 13:09
@xuxiaowei-com-cn

Copy link
Copy Markdown
Collaborator

I have read the CLA Document and I hereby sign the CLA

  • But there might be issues in some places.
  • Please sign again, and after signing, click recheck.

@EvanYao826

Copy link
Copy Markdown
Author

Thanks for the heads up! I have rechecked and signed the CLA. Please let me know if there are any further issues.

@xuxiaowei-com-cn

Copy link
Copy Markdown
Collaborator

Thanks for the heads up! I have rechecked and signed the CLA. Please let me know if there are any further issues.

It has been found that you have not signed the CLA. After signing, the CLA will appear green instead of the current yellow.

@EvanYao826

Copy link
Copy Markdown
Author

I have rechecked and signed the CLA. The status should be updated now. Please let me know if there are any other issues. @xuxiaowei-com-cn

@xuxiaowei-com-cn

Copy link
Copy Markdown
Collaborator

I have rechecked and signed the CLA. The status should be updated now. Please let me know if there are any other issues. @xuxiaowei-com-cn

After signing, the CLA will appear green instead of the current yellow.

image

@EvanYao826

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

@xuxiaowei-com-cn I have signed the CLA again. The commit email is correctly set to the noreply format (155432245+EvanYao826@users.noreply.github.com). Could you please check if the CLA status has been updated? If it still shows as not signed, I may need to click the recheck link again.

@xuxiaowei-com-cn

Copy link
Copy Markdown
Collaborator

I have read the CLA Document and I hereby sign the CLA

@xuxiaowei-com-cn I have signed the CLA again. The commit email is correctly set to the noreply format (155432245+EvanYao826@users.noreply.github.com). Could you please check if the CLA status has been updated? If it still shows as not signed, I may need to click the recheck link again.

After you have completed the signing process, please click recheck.

@xuxiaowei-com-cn

Copy link
Copy Markdown
Collaborator

GitHub Actions run has been approved.

@uuuyuqi

uuuyuqi commented May 25, 2026

Copy link
Copy Markdown
Collaborator

Hi @EvanYao826, thanks for the PR. This overlaps with #4319, which was opened earlier and also fixes #4296, while additionally covering a few related consistency concerns (atomic read-and-remove of the snapshot, distinguishing an empty snapshot from a missing one, namespace-aware key, plus unit tests). We're going to move forward with #4319 to close out the issue, so this PR will be closed in favor of that one.

Before submitting future PRs, a quick scan of open PRs and issue comments helps avoid duplicate work — but the effort here is appreciated regardless.

@EvanYao826 EvanYao826 force-pushed the fix/nacos-config-data-loader-snapshot branch from 868ea6a to 4522b14 Compare May 25, 2026 11:31
@EvanYao826

Copy link
Copy Markdown
Author

Rebased onto latest 2025.1.x and resolved merge conflicts. The upstream already merged #4319 with a similar config snapshot fix — my PR now builds on top of that, adding the isEmpty() null-safety check and a debug log for snapshot cache hits.

@EvanYao826

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

@EvanYao826

Copy link
Copy Markdown
Author

Hi @uuuyuqi, thanks for the review! You're right that this overlaps with #4319. After rebasing onto the latest 2025.1.x, the upstream already merged #4319's config snapshot fix. My PR now adds incremental improvements on top:

  1. isEmpty() null-safety check for the snapshot
  2. Debug-level logging for snapshot cache hits

If these additions are useful, I'm happy to keep this PR open. Otherwise, feel free to close it if the team considers #4319 sufficient. Let me know!

@EvanYao826

Copy link
Copy Markdown
Author

Hi, friendly ping — CLA is signed and all CI checks are passing. Could a maintainer take a look when available? Thanks!

@uuuyuqi

uuuyuqi commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Hi @EvanYao826, I checked the updated diff after the rebase.

The debug log itself is fine, but adding config.isEmpty() is not a null-safety improvement. It changes the behavior intentionally introduced by #4319:

  • null means no snapshot exists, so falling back to ConfigService#getConfig(...) is appropriate.
  • An empty string means a snapshot exists and the pushed configuration was intentionally cleared. In that case, the empty snapshot must be used without fetching remote config.

If an empty snapshot falls back to a Nacos node that has not yet synchronized, the loader may read the previous non-empty configuration again. This reintroduces the stale-config scenario that the snapshot mechanism is intended to prevent.

The latest CI run also confirms this regression through the following failing test:

NacosConfigDataLoaderTest.loadWhenSnapshotIsEmptyThenDoesNotUseRemoteConfig

Since #4319 already implements the original fix, please remove the isEmpty() fallback. After that, the only remaining change would be the debug log, which would be better handled separately if the project needs it. As currently written, this PR should not be merged.

@EvanYao826

Copy link
Copy Markdown
Author

@uuuyuqi Thanks for the detailed review and explanation! You're absolutely right — the isEmpty() check changes the semantics of #4319's snapshot mechanism. An empty string snapshot should be treated as "config intentionally cleared" and must not fall back to remote config.

I'll remove the isEmpty() fallback immediately. Since #4319 already implements the core fix, the only remaining change in this PR would be the debug log for snapshot cache hits. Would you prefer I:

  1. Strip this PR down to just the debug log addition, or
  2. Close this PR entirely since fix: use Nacos config snapshot in ConfigData loader #4319 covers the fix?

Working on removing the isEmpty() change now.

@uuuyuqi

uuuyuqi commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the quick update. Please go with option 1 and keep this PR focused on the debug log addition only.

I checked the latest diff, and it now only adds the snapshot-hit debug log, which is the right direction. Please also clean up the commit history and PR metadata so this PR no longer presents itself as the fix for #4296, since that was already handled by #4319.

A single focused commit would be clearer, for example:

chore: add debug log for Nacos config snapshot hits

Please also update the PR title and description accordingly, and remove the Fixes #4296 wording. After that I can take another look.

Add a debug-level log message when a Nacos config value is loaded from
the local snapshot cache instead of being fetched from the remote server.
This aids troubleshooting in cluster mode where snapshot cache hits
indicate a config change notification arrived before the remote node synced.
@EvanYao826 EvanYao826 force-pushed the fix/nacos-config-data-loader-snapshot branch from ca32971 to a836ad3 Compare June 18, 2026 13:34
@EvanYao826 EvanYao826 changed the title fix: use config snapshot in NacosConfigDataLoader to fix stale config in cluster mode chore: add debug log for Nacos config snapshot hits Jun 18, 2026
@EvanYao826

Copy link
Copy Markdown
Author

Hi @uuuyuqi, I've cleaned up this PR as requested:

  1. Single commit: chore: add debug log for Nacos config snapshot hits — squashed into one focused commit on top of the latest 2025.1.x.
  2. Only the debug log remains: No isEmpty() check, no behavioral change — just the snapshot cache hit log.
  3. Updated PR title and description: Removed the Fixes #4296 claim, since fix: use Nacos config snapshot in ConfigData loader #4319 already handles the core fix.

Ready for another look when you have time. Thanks!

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.

4 participants