[client] Add validation for client.scanner.log.max-poll-records and client.connect-timeout - #3069
Conversation
…lient.connect-timeout
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@Prajwal-banakar Ty for the PR, left some comments, PTAL
|
|
||
| public static void validateClientConfigs(Configuration conf) { | ||
| int maxPollRecords = conf.getInt(ConfigOptions.CLIENT_SCANNER_LOG_MAX_POLL_RECORDS); | ||
| if (maxPollRecords <= 0) { |
There was a problem hiding this comment.
Shall we use validMinValue?
There was a problem hiding this comment.
Good catch, thanks! Switched to validMinValue
| } | ||
|
|
||
| long connectTimeoutMs = conf.get(ConfigOptions.CLIENT_CONNECT_TIMEOUT).toMillis(); | ||
| if (connectTimeoutMs <= 0) { |
There was a problem hiding this comment.
mb we can have validMinDuration(conf, option, minMillis) helper to match the existing pattern?
There was a problem hiding this comment.
added a validMinDuration helper
| Configuration.fromMap( | ||
| extractPrefix(new HashMap<>(conf.toMap()), CLIENT_PREFIX + "fs.")), | ||
| null); | ||
| FlussConfigUtils.validateClientConfigs(conf); |
There was a problem hiding this comment.
Nit: can we move FlussConfigUtils.validateClientConfigs(conf) to the very first line of the constructor, before FileSystem.initialize(...) and setupClientMetricsConfiguration()?
Right now a bad config still initializes the filesystem and sets up metrics before failing, while fail-fast is cheaper and avoids partial side effects.
There was a problem hiding this comment.
updated to validate at the beginning of the constructor
|
|
||
| // connect-timeout = 0 should fail | ||
| Configuration zeroTimeoutConf = new Configuration(); | ||
| zeroTimeoutConf.set(ConfigOptions.CLIENT_CONNECT_TIMEOUT, java.time.Duration.ZERO); |
There was a problem hiding this comment.
nit: why fully qualified name?
There was a problem hiding this comment.
replaced with import
|
Hi @fresh-borzoni adressed all the comments, could you please take another look when you get a chance? thanks! |
|
Hi @fresh-borzoni PTAL another look here when you have some time, Thanks! |
2 participants
Purpose
Linked issue: close #3068
Add client config validation for
client.scanner.log.max-poll-recordsandclient.connect-timeoutduringFlussConnectioninitialization. Setting either of these to 0 or negative makes no practical sense — poll() would return nothing and TCP connect would time out immediately.Brief change log
validateClientConfigs()inFlussConfigUtilscoveringclient.scanner.log.max-poll-records(must be > 0) andclient.connect-timeout(must be > 0)validateClientConfigs()inFlussConnectionconstructorFlussConfigUtilsTestTests
Existing
FlussConfigUtilsTestextended withtestValidateClientConfigs()covering default validity and each invalid case.API and Format
NO
Documentation
NO