Skip to content

Conversation

@wangmm0220
Copy link
Contributor

Description

Please briefly describe the code changes in this pull request.

Checklist

Please check the items in the checklist if applicable.

  • Is the user manual updated?
  • Are the test cases passed and automated?
  • Is there no significant decrease in test coverage?

@wangmm0220 wangmm0220 merged commit 5c02c8f into enh/addStreamLatency Nov 10, 2025
1 check passed
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @wangmm0220, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces significant improvements to the system's caching mechanisms, particularly for stable tag filters and statistical information. The changes aim to provide better insights into cache performance through new metrics, enhance memory safety by refining LRU cache handle management and pointer ownership, and improve debugging capabilities with more verbose logging. These updates contribute to a more robust and observable caching infrastructure.

Highlights

  • Enhanced Tag Filter Cache Metrics: New fields numTagDataEntries, accTimes (now uint64_t), and hitTimes (new, uint64_t) have been added to the stable tag filter cache structures to provide more granular statistics on cache usage and efficiency.
  • Improved LRU Cache Management: The getStatisInfoFromCache function now returns an LRUHandle pointer, and a new releaseCacheHandle function is introduced, ensuring proper reference counting and explicit release of LRU cache entries, preventing potential resource leaks.
  • Refined Tag Condition Handling: The pTagColIds parameter in putStableCachedTableList and metaStableTagFilterCachePut has been changed to SArray**, allowing the function to take ownership of the SArray pointer and set it to NULL after use, improving memory management.
  • Comprehensive Error Handling and Logging: Added explicit memory deallocation for pTagColIds and pTagCondKey in error paths, and enhanced debug/info logging with more detailed cache operation information, including tag condition strings and entry counts.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several valuable improvements, primarily focused on enhancing stream latency monitoring and fixing caching-related bugs. Key changes include adding more detailed logging and metrics for cache performance, which will significantly improve observability. A critical bug fix addresses a potential use-after-free issue by refining the management of LRU cache handles. Additionally, the code now correctly uses the stable tag filter cache's access times for metrics, fixing a notable bug. The ownership semantics for certain data structures have been clarified by using pointer-to-pointers, which helps prevent memory leaks. Overall, these are solid improvements to the stability and maintainability of the codebase. I've identified a memory leak and a couple of areas where error handling could be made more robust.

Comment on lines 2245 to +2247
taosArrayDestroy(pUidList);
taosArrayDestroy(pTagColIds);
taosMemFreeClear(pTagCondKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The variable pTagCondStr, allocated on line 2098, is not freed in any execution path, leading to a memory leak. It should be freed in the _error cleanup block.

  taosArrayDestroy(pUidList);
  taosArrayDestroy(pTagColIds);
  taosMemFreeClear(pTagCondKey);
  taosMemFreeClear(pTagCondStr);

Comment on lines +906 to +915
metaInfo("vgId:%d, suid:%" PRIu64 " new tag data filter entry added"
"(stable num:%d, total tag data entries num:%" PRIu32 "); "
"this tag condition data entries num:%d, "
"current stable tag conditions num:%d, "
"total tag data entries num:%" PRIu32,
vgId, suid, (int32_t)taosHashGetSize(pTableEntry),
pTagConds ? (int32_t)taosHashGetSize((*pTagConds)->set) : 0);
pMeta->pCache->sStableTagFilterResCache.numTagDataEntries,
pTagConds ? (int32_t)taosHashGetSize((*pTagConds)->set) : 0,
pFilterEntry ? (int32_t)taosHashGetSize((*pFilterEntry)->set) : 0,
(*pTagConds)->numTagDataEntries);
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The log message has "total tag data entries num" twice, referring to two different metrics (total in cache vs. total in the current stable table). This is confusing. The second instance should be renamed for clarity to distinguish between the total for the entire cache and the total for the current stable table.

    metaInfo("vgId:%d, suid:%" PRIu64 " new tag data filter entry added"
      "(stable num:%d, total tag data entries num:%" PRIu32 "); "
      "this tag condition data entries num:%d, "
      "current stable tag conditions num:%d, "
      "current stable total tag data entries num:%" PRIu32,
      vgId, suid, (int32_t)taosHashGetSize(pTableEntry),
      pMeta->pCache->sStableTagFilterResCache.numTagDataEntries,
      pTagConds ? (int32_t)taosHashGetSize((*pTagConds)->set) : 0,
      pFilterEntry ? (int32_t)taosHashGetSize((*pFilterEntry)->set) : 0,
      (*pTagConds)->numTagDataEntries);

Comment on lines +1416 to +1418
if (p1 == NULL) {
return terrno;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Returning the global terrno when taosArrayGet returns NULL is fragile. The value of terrno might be stale or not set by taosArrayGet, leading to an incorrect error code being propagated. It's more robust to return a specific error code.

      if (p1 == NULL) {
        return TSDB_CODE_INTERNAL_ERROR;
      }

Comment on lines +1424 to +1426
if (p2 == NULL) {
return terrno;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Similar to the previous comment, returning the global terrno is fragile. It's better to return a specific error code for robustness.

      if (p2 == NULL) {
        return TSDB_CODE_INTERNAL_ERROR;
      }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants