Conversation
| } | ||
| } | ||
|
|
||
| public void startKeepAlivePeriodically(int keepAliveIntervalMs) { |
There was a problem hiding this comment.
Where do we plan to start to start keepalives? This method seems currently unused.
Separately, I wonder if we're giving clients too much control over holding server resources with keepalives. I can see issue where clients forget to close and server starts leaking iterators.
Should we simplify and get rid of keepalives and refresh iterator TTL on server side whenever a ScanKvRequest is made?
It's a two-way door to release this feature without keepalive. We cannot easily remove keepalive mechanism once added as it would break clients who rely on keepalive mechanism.
| * Creates a new {@link SnapshotQuery} for this table to configure and execute a snapshot query | ||
| * to read all current data in a table bucket (requires to be a Primary Key Table). | ||
| */ | ||
| SnapshotQuery newSnapshotQuery(); |
There was a problem hiding this comment.
Nit: SnapshotQuery sounds like querying an existing snapshot. I cannot think of any name though.. maybe just Query?
| public void keepAlive(byte[] scannerId) { | ||
| ScannerContext context = scanners.get(ByteBuffer.wrap(scannerId)); | ||
| if (context != null) { | ||
| context.updateLastAccessTime(clock.milliseconds()); |
There was a problem hiding this comment.
Is this thread safe? What if the context is cleaned right after you retrieve it from the map?
There was a problem hiding this comment.
I think every scanner should have an access lock protection, just like what kudu does: https://github.com/apache/kudu/blob/master/src/kudu/tserver/scanners.h#L384
| this.scannerTtlMs = conf.get(ConfigOptions.SERVER_SCANNER_TTL).toMillis(); | ||
| this.cleanupExecutor = | ||
| Executors.newSingleThreadScheduledExecutor( | ||
| new ExecutorThreadFactory("scanner-cleanup-thread")); |
There was a problem hiding this comment.
Can we reuse the scheduler defined in TabletServer?
3 participants
No description provided.