Skip to content

Conversation

@beryllw
Copy link
Contributor

@beryllw beryllw commented Dec 12, 2025

Purpose

Linked issue: close #2111

Brief change log

Tests

API and Format

Documentation

@beryllw beryllw marked this pull request as draft December 12, 2025 08:14
@beryllw beryllw force-pushed the auto-inc-impl branch 2 times, most recently from 45dc38a to 4d674d7 Compare December 12, 2025 15:04
@vamossagar12
Copy link
Contributor

hi, I had created this Pr: #2119 for this issue. Do we want to prefer this one over that?

@xx789633
Copy link
Contributor

Hi @vamossagar12 , we prefer this one to work on since it is more comprehensive.

@beryllw beryllw force-pushed the auto-inc-impl branch 2 times, most recently from 19d0475 to 697dbcf Compare December 24, 2025 04:33
@beryllw beryllw marked this pull request as ready for review December 24, 2025 06:03
@beryllw beryllw requested a review from xx789633 December 24, 2025 06:10
new Column(
column.getName(),
column.getDataType().copy(false),
column.getDataType().copy(true),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Partial update is required, so the data type here needs to be temporarily set to nullable.

@beryllw beryllw changed the title [kv]: Add an implementation of AutoIncIDBuffer [kv] Add an implementation of AutoIncIDBuffer Dec 25, 2025
@beryllw beryllw requested a review from xx789633 December 25, 2025 03:16
@beryllw beryllw requested a review from xx789633 December 26, 2025 01:13
Copy link
Contributor

@platinumhamburg platinumhamburg left a comment

Choose a reason for hiding this comment

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

@beryllw Thanks for contribution, I left some comments.

@platinumhamburg
Copy link
Contributor

In addition to integration tests, I think we should also add several unit tests in KvTabletTest to cover behavior assertions after a TabletServer restart and after a single segment overflows.

@beryllw
Copy link
Contributor Author

beryllw commented Dec 31, 2025

In addition to integration tests, I think we should also add several unit tests in KvTabletTest to cover behavior assertions after a TabletServer restart and after a single segment overflows.

We could create a new issue to address the failover problem, including the design and implementation of its corresponding recovery mechanisms.

@beryllw beryllw marked this pull request as draft January 3, 2026 14:27
@beryllw beryllw marked this pull request as ready for review January 6, 2026 11:16
@xx789633 xx789633 requested review from wuchong and removed request for platinumhamburg January 7, 2026 02:22
Copy link
Contributor

@platinumhamburg platinumhamburg left a comment

Choose a reason for hiding this comment

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

I've left some additional comments.

…alone interface and removing stateful coupling in AutoIncProcessor
Copy link
Contributor

@platinumhamburg platinumhamburg left a comment

Choose a reason for hiding this comment

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

LGTM, @wuchong Do you have any other suggestions?

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.

[kv/auto increment column] Implement AutoIncIDBuffer on tablet server

4 participants