[common] Fix NumberFormatException when parsing empty string properties #2372

sushantmane · 2026-01-13T00:24:32Z

[common] Fix NumberFormatException when parsing empty string properties

Modified VeniceProperties numeric parsing methods to handle empty strings gracefully: -
getLong/getInt/getDouble/getSizeInBytes methods with default values now return the default
when property value is empty or whitespace-only - Methods without default values throw
descriptive VeniceException instead of NumberFormatException - getOptionalInt returns
Optional.empty() for empty values

Code changes

Added new code behind a config. If so list the config names and their default values in the PR description.
Introduced new log lines.
- Confirmed if logs need to be rate limited to avoid excessive logging.

Concurrency-Specific Checks

Both reviewer and PR author to verify

Code has no race conditions or thread safety issues.
Proper synchronization mechanisms (e.g., synchronized, RWLock) are used where needed.
No blocking calls inside critical sections that could lead to deadlocks or performance degradation.
Verified thread-safe collections are used (e.g., ConcurrentHashMap, CopyOnWriteArrayList).
Validated proper exception handling in multi-threaded code to avoid silent thread termination.

How was this PR tested?

New unit tests added.
New integration tests added.
Modified or extended existing tests.
Verified backward compatibility (if applicable).

Does this PR introduce any user-facing or breaking changes?

No. You can skip the rest of this section.
Yes. Clearly explain the behavior change and its impact.

Copilot

Pull request overview

This PR aims to fix NumberFormatException when parsing empty string properties in VeniceProperties. The changes modify numeric parsing methods (getLong, getInt, getDouble, getSizeInBytes, and getOptionalInt) to handle empty or whitespace-only strings gracefully by either returning default values or throwing descriptive VeniceException instead of NumberFormatException. The PR also includes comprehensive test coverage for these scenarios and unrelated logging level changes in AsyncStoreChangeNotifier.

Changes:

Modified VeniceProperties numeric parsing methods to check for empty/whitespace-only values before parsing
Added comprehensive test coverage for empty string handling in all affected methods
Changed logging level from info to debug in AsyncStoreChangeNotifier (unrelated to main purpose)

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 12 comments.

File	Description
internal/venice-client-common/src/main/java/com/linkedin/venice/utils/VeniceProperties.java	Added empty string checks to getLong, getInt, getDouble, getSizeInBytes, and getOptionalInt methods
internal/venice-client-common/src/test/java/com/linkedin/venice/utils/VenicePropertiesTest.java	Added comprehensive test cases for empty string handling in numeric parsing methods
internal/venice-common/src/main/java/com/linkedin/venice/meta/AsyncStoreChangeNotifier.java	Changed logging level from info to debug for task registration and execution messages

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/venice-client-common/src/main/java/com/linkedin/venice/utils/VeniceProperties.java

internal/venice-common/src/main/java/com/linkedin/venice/meta/AsyncStoreChangeNotifier.java

internal/venice-client-common/src/main/java/com/linkedin/venice/utils/VeniceProperties.java

internal/venice-common/src/main/java/com/linkedin/venice/meta/AsyncStoreChangeNotifier.java

internal/venice-client-common/src/main/java/com/linkedin/venice/utils/VeniceProperties.java

Modified VeniceProperties numeric parsing methods to handle empty strings gracefully: - getLong/getInt/getDouble/getSizeInBytes methods with default values now return the default when property value is empty or whitespace-only - Methods without default values throw descriptive VeniceException instead of NumberFormatException - getOptionalInt returns Optional.empty() for empty values Added comprehensive TestNG tests covering all modified methods with empty string, whitespace-only, valid, and missing property scenarios.

- Fix whitespace handling bug in numeric parsing methods by trimming values before passing to parse methods (Long.parseLong, Integer.parseInt, Double.parseDouble, convertSizeFromLiteral) - Add comprehensive test coverage for values with leading/trailing whitespace - Revert unrelated logging level changes in AsyncStoreChangeNotifier - Fix SpotBugs CNT_ROUGH_CONSTANT_VALUE violation by changing test value from 3.14 to 3.15 All tests pass and SpotBugs checks pass with 0 violations.

Previously each method called trim() twice: - Once in the isEmpty check: value.trim().isEmpty() - Once for parsing: Long.parseLong(value.trim()) Now each method: 1. Checks for null first 2. Calls trim() once and stores in trimmedValue variable 3. Checks if trimmedValue is empty 4. Parses trimmedValue This reduces string operations and improves performance while maintaining the same behavior.

internal/venice-client-common/src/main/java/com/linkedin/venice/utils/VeniceProperties.java

majisourav99 · 2026-01-13T20:59:36Z

internal/venice-client-common/src/main/java/com/linkedin/venice/utils/VeniceProperties.java

  public long getLong(String name, long defaultValue) {
    if (containsKey(name)) {
-      return Long.parseLong(get(name));
+      String value = get(name);


String value = props.get(name);
if (value == null) {
return defaultValue;
}
....

sushantmane · 2026-01-13T21:10:39Z

Thanks @arjun4084346 & @majisourav99 for the review!

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

majisourav99 · 2026-01-14T00:23:11Z

internal/venice-client-common/src/main/java/com/linkedin/venice/utils/VeniceProperties.java

-    if (containsKey(name)) {
-      return Long.parseLong(get(name));
-    } else {
+    if (!containsKey(name)) {


why double checking containsKey? you already check it in getTrimmedValueOrNull

Addresses review feedback by eliminating redundant containsKey checks in getTrimmedValueOrNull method. Changed from using get() method (which internally checks containsKey) to directly using props.get(), reducing map lookups from 3 to 1 in the happy path. Also deferred containsKey checks in getLong/getInt/ getDouble/getSizeInBytes methods to only execute when distinguishing between missing keys and empty values.

arjun4084346 · 2026-01-14T19:07:45Z

internal/venice-client-common/src/main/java/com/linkedin/venice/utils/VeniceProperties.java

-    } else {
-      throw new UndefinedPropertyException(name);
+    String trimmedValue = getTrimmedValueOrNull(name);
+    if (trimmedValue == null) {


sorry for minor comments; but i think L365-L368 can also go inside method getTrimmedValueOrNull and it can be renamed like getTrimmedNonEmptyValueOrThrowException

manujose0 · 2026-01-15T05:53:06Z

From Claude Code PR Review

⏺ Pull Request Review: #2372

Title: [common] Fix NumberFormatException when parsing empty string properties

Summary

This PR fixes a bug where empty string property values cause NumberFormatException when parsing numeric values in VeniceProperties.java. The fix centralizes empty/null handling across all numeric getter methods.

Changes Overview

Files Modified: 3 files (+61/-49 lines)

VeniceProperties.java - Core logic changes
VenicePropertiesTest.java - Test coverage
TestChangelogConsumer.java - Integration tests

What I Like ✓

Smart Refactoring: Introduces getTrimmedValueOrNull() helper that centralizes the logic for handling null/empty/whitespace strings across 6+ numeric parsing methods
Consistent Behavior: All numeric getters (getLong(), getInt(), getDouble(), getSizeInBytes()) now handle empty strings uniformly
Clear Error Semantics:
- Missing properties → UndefinedPropertyException
- Empty/whitespace properties → VeniceException
- This distinction is valuable for debugging
Performance Optimization: Final commit removes redundant containsKey() checks that would cause duplicate map lookups
Good Test Coverage: Tests include edge cases like whitespace-only strings

Potential Concerns / Questions

Breaking Change?: The behavior change for empty strings (now throws exception vs previously possibly using default) could affect existing deployments. Was this considered? Are there callers relying on the previous behavior?
Error Messages: When throwing VeniceException for empty strings, does the error message clearly indicate that an empty string was the problem? This would help operators diagnose misconfigurations.
Null vs Empty Semantics: Should empty strings be treated as "not provided" and use defaults, or is throwing an exception the right choice? The current implementation treats empty as invalid, which seems reasonable but worth documenting.
getSizeInBytes() lowercase conversion: The change to convert to lowercase after trimming is good, but verify that all size literals are case-insensitive in the docs.

Code Quality: 8.5/10

This is a solid defensive programming fix that improves robustness. The refactoring reduces code duplication and makes the codebase more maintainable. The main consideration is ensuring this behavior change doesn't break existing configurations.

Recommendation: ✅ Approve with minor consideration for communication about the behavior change in release notes.

Copilot AI review requested due to automatic review settings January 13, 2026 00:24

Copilot started reviewing on behalf of sushantmane January 13, 2026 00:24 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

sushantmane added 3 commits January 13, 2026 11:32

sushantmane force-pushed the fix-number-format-exception-in-vpj branch from b5f6211 to 0ef4ee9 Compare January 13, 2026 19:32

arjun4084346 reviewed Jan 13, 2026

View reviewed changes

internal/venice-client-common/src/main/java/com/linkedin/venice/utils/VeniceProperties.java Outdated Show resolved Hide resolved

arjun4084346 reviewed Jan 13, 2026

View reviewed changes

internal/venice-client-common/src/main/java/com/linkedin/venice/utils/VeniceProperties.java Outdated Show resolved Hide resolved

majisourav99 reviewed Jan 13, 2026

View reviewed changes

Address review comments

861f7e5

Copilot AI review requested due to automatic review settings January 13, 2026 22:11

Copilot started reviewing on behalf of sushantmane January 13, 2026 22:11 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

majisourav99 reviewed Jan 14, 2026

View reviewed changes

sushantmane added 2 commits January 14, 2026 01:09

Fix flaky tests

a119b64

arjun4084346 reviewed Jan 14, 2026

View reviewed changes

[common] Fix NumberFormatException when parsing empty string properties #2372

Are you sure you want to change the base?

[common] Fix NumberFormatException when parsing empty string properties #2372

Uh oh!

Conversation

sushantmane commented Jan 13, 2026