Skip to content

Fix retry handling in executeSelect() - #5501

Open
dimas-b wants to merge 1 commit into
apache:mainfrom
dimas-b:fix-select-retries
Open

Fix retry handling in executeSelect()#5501
dimas-b wants to merge 1 commit into
apache:mainfrom
dimas-b:fix-select-retries

Conversation

@dimas-b

@dimas-b dimas-b commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Do not accumulate result entries across retries.
Convert each result set stream into a fresh list of results.

Fixes #5498

Checklist

  • 🛡️ Don't disclose security issues! (contact security@apache.org)
  • 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
  • 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
  • 💡 Added comments for complex logic
  • 🧾 Updated CHANGELOG.md (if needed)
  • 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

@github-project-automation github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Sep 11, 2026
@dimas-b
dimas-b force-pushed the fix-select-retries branch 2 times, most recently from df88417 to a890feb Compare September 11, 2026 19:41
Do not accumulate result entries across retries.
Convert each result set stream into a fresh list of results.

Fixes apache#5498

@flyrain flyrain left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 Thanks for the fix, @dimas-b !

ArrayList<T> results = new ArrayList<>();
executeSelectOverStream(query, converterInstance, stream -> stream.forEach(results::add));
return results;
AtomicReference<List<T>> results = new AtomicReference<>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: it is a local variable, would ArrayList be good enough?

@dimas-b dimas-b Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, @flyrain , what do you mean by "good enough"? Stream.toList() (below) may produce a different List impl. class.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm just confirm if AtomicReference is necessary. It is a nit anyway. Feel free to ignore it.

@github-project-automation github-project-automation Bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Sep 11, 2026
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.

DatasourceOperations.executeSelect() handles retries incorrectly

2 participants