Skip to content

nacos托管 proxy druid动态数据源 example提交 - #4045

Open
shiyiyue1102 wants to merge 3 commits into
alibaba:2023.xfrom
shiyiyue1102:2023.x
Open

nacos托管 proxy druid动态数据源 example提交#4045
shiyiyue1102 wants to merge 3 commits into
alibaba:2023.xfrom
shiyiyue1102:2023.x

Conversation

@shiyiyue1102

Copy link
Copy Markdown
Collaborator

Describe what this PR does / why we need it

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

@shiyiyue1102
shiyiyue1102 requested a review from chickenlj August 12, 2025 05:39

@oss-sentinel-ai oss-sentinel-ai left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Summary

This PR adds a new nacos-druid-example module demonstrating Nacos config management with Druid dynamic datasource. The example is useful for the community, but the code has several issues that should be addressed before merging.

Review Notes

  • [Critical] Resource leak in DemoController constructor — The constructor calls scheduleTask() 10 times in a loop, and each call creates a new ScheduledThreadPoolExecutor via Executors.newScheduledThreadPool(2). This creates 10 thread pools (20 threads total) that are never shut down. Consider using a single shared executor and managing its lifecycle properly (e.g., implement DisposableBean or use @PreDestroy).

  • [Warning] System.out.println usage — The example uses System.out.println for output. Consider using SLF4J logging (LoggerFactory.getLogger(...)) to demonstrate best practices, even in example code.

  • [Warning] Constructor doing too much work — The constructor schedules 10 tasks immediately. This is unusual for a Spring controller and may cause issues during application startup. Consider using @PostConstruct or ApplicationRunner instead.

  • [Info] Minor: 123l123L — In selectTask(), jdbcEntityRepository.findNameById(123l) uses lowercase l which can be confused with 1. Use uppercase L for clarity.

  • [Info] Package naming — The package com.alibaba.demo.nacosdruidexample is fine, but consider aligning with the existing example package structure (e.g., com.alibaba.cloud.examples.nacosdruid).

Suggestions

  1. Fix the resource leak (most important)
  2. Add proper logging
  3. Move task scheduling out of the constructor
  4. Add a README explaining how to run the example and what it demonstrates

Automated review by github-manager-bot

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.

2 participants