Skip to content

Implement InsertTabletsWithCtx for tablet insertion#156

Open
hchw wants to merge 1 commit intoapache:mainfrom
hchw:main
Open

Implement InsertTabletsWithCtx for tablet insertion#156
hchw wants to merge 1 commit intoapache:mainfrom
hchw:main

Conversation

@hchw
Copy link
Copy Markdown

@hchw hchw commented Mar 16, 2026

Add InsertTabletsWithCtx method to insert multiple tablets with context support.

Add InsertTabletsWithCtx method to insert multiple tablets with context support.
@HTHou
Copy link
Copy Markdown
Contributor

HTHou commented Mar 18, 2026

Any motivation to add this function?

@hchw
Copy link
Copy Markdown
Author

hchw commented Mar 18, 2026

Any motivation to add this function?

To add a timeout to data writes, preventing function handles from being released because of write timeouts.

@hchw
Copy link
Copy Markdown
Author

hchw commented Mar 27, 2026

Why don't the functions you provide come with timeout control parameters?

@HTHou
Copy link
Copy Markdown
Contributor

HTHou commented Apr 27, 2026

Thanks for the PR. I think the motivation is valid: the current InsertTablets path always uses context.Background(), while the generated thrift client already accepts a context.Context, so callers currently have no way to set a deadline or cancel a long-running write RPC.

That said, this looks like a partial API fix rather than a complete one. Other write methods such as InsertTablet, InsertAlignedTablet, InsertAlignedTablets, and InsertRecords still use context.Background() internally, so users would only get timeout/cancellation support for one insertion path. It may be better to add WithContext variants consistently for the main write APIs, with the existing methods delegating to them using context.Background().

One more detail worth documenting: this timeout would be a client-side RPC context timeout/cancellation. It lets the caller stop waiting and release client-side resources, but it does not necessarily guarantee that a request already received by the server is aborted server-side.

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