Skip to content

Commit b5f6211

Browse files
sushantmanesushantmane
committed
[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 Added comprehensive TestNG tests covering all modified methods with empty string, whitespace-only, valid, and missing property scenarios.
1 parent a1f3bf8 commit b5f6211

File tree

3 files changed

+236
-10
lines changed

3 files changed

+236
-10
lines changed

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

Lines changed: 43 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -341,63 +341,95 @@ public boolean getBoolean(String key) {
341341

342342
public long getLong(String name, long defaultValue) {
343343
if (containsKey(name)) {
344-
return Long.parseLong(get(name));
344+
String value = get(name);
345+
if (value == null || value.trim().isEmpty()) {
346+
return defaultValue;
347+
}
348+
return Long.parseLong(value);
345349
} else {
346350
return defaultValue;
347351
}
348352
}
349353

350354
public long getLong(String name) {
351355
if (containsKey(name)) {
352-
return Long.parseLong(get(name));
356+
String value = get(name);
357+
if (value == null || value.trim().isEmpty()) {
358+
throw new VeniceException("Property " + name + " is defined but has an empty value");
359+
}
360+
return Long.parseLong(value);
353361
} else {
354362
throw new UndefinedPropertyException(name);
355363
}
356364
}
357365

358366
public int getInt(String name, int defaultValue) {
359367
if (containsKey(name)) {
360-
return Integer.parseInt(get(name));
368+
String value = get(name);
369+
if (value == null || value.trim().isEmpty()) {
370+
return defaultValue;
371+
}
372+
return Integer.parseInt(value);
361373
} else {
362374
return defaultValue;
363375
}
364376
}
365377

366378
public int getInt(String name) {
367379
if (containsKey(name)) {
368-
return Integer.parseInt(get(name));
380+
String value = get(name);
381+
if (value == null || value.trim().isEmpty()) {
382+
throw new VeniceException("Property " + name + " is defined but has an empty value");
383+
}
384+
return Integer.parseInt(value);
369385
} else {
370386
throw new UndefinedPropertyException(name);
371387
}
372388
}
373389

374390
public Optional<Integer> getOptionalInt(String name) {
375391
if (containsKey(name)) {
376-
return Optional.of(Integer.parseInt(get(name)));
392+
String value = get(name);
393+
if (value == null || value.trim().isEmpty()) {
394+
return Optional.empty();
395+
}
396+
return Optional.of(Integer.parseInt(value));
377397
} else {
378398
return Optional.empty();
379399
}
380400
}
381401

382402
public double getDouble(String name, double defaultValue) {
383403
if (containsKey(name)) {
384-
return Double.parseDouble(get(name));
404+
String value = get(name);
405+
if (value == null || value.trim().isEmpty()) {
406+
return defaultValue;
407+
}
408+
return Double.parseDouble(value);
385409
} else {
386410
return defaultValue;
387411
}
388412
}
389413

390414
public double getDouble(String name) {
391415
if (containsKey(name)) {
392-
return Double.parseDouble(get(name));
416+
String value = get(name);
417+
if (value == null || value.trim().isEmpty()) {
418+
throw new VeniceException("Property " + name + " is defined but has an empty value");
419+
}
420+
return Double.parseDouble(value);
393421
} else {
394422
throw new UndefinedPropertyException(name);
395423
}
396424
}
397425

398426
public long getSizeInBytes(String name, long defaultValue) {
399427
if (containsKey(name)) {
400-
return getSizeInBytes(name);
428+
String value = get(name);
429+
if (value == null || value.trim().isEmpty()) {
430+
return defaultValue;
431+
}
432+
return convertSizeFromLiteral(value);
401433
} else {
402434
return defaultValue;
403435
}
@@ -409,6 +441,9 @@ public long getSizeInBytes(String name) {
409441
}
410442

411443
String bytes = get(name);
444+
if (bytes == null || bytes.trim().isEmpty()) {
445+
throw new VeniceException("Property " + name + " is defined but has an empty value");
446+
}
412447
return convertSizeFromLiteral(bytes);
413448
}
414449

‎internal/venice-client-common/src/test/java/com/linkedin/venice/utils/VenicePropertiesTest.java‎

Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,4 +200,195 @@ public void testMapToString() {
200200
String emptyResult = VeniceProperties.mapToString(emptyMap);
201201
assertEquals(emptyResult, "");
202202
}
203+
204+
@Test
205+
public void testGetLongWithEmptyString() {
206+
// Test getLong with default value when property is empty string
207+
Properties properties = new Properties();
208+
properties.put("empty.long", "");
209+
properties.put("whitespace.long", " ");
210+
properties.put("valid.long", "12345");
211+
VeniceProperties veniceProperties = new VeniceProperties(properties);
212+
213+
// Empty string should return default value
214+
assertEquals(veniceProperties.getLong("empty.long", 999L), 999L);
215+
216+
// Whitespace-only string should return default value
217+
assertEquals(veniceProperties.getLong("whitespace.long", 888L), 888L);
218+
219+
// Valid value should be parsed
220+
assertEquals(veniceProperties.getLong("valid.long", 999L), 12345L);
221+
222+
// Missing property should return default value
223+
assertEquals(veniceProperties.getLong("missing.long", 777L), 777L);
224+
}
225+
226+
@Test
227+
public void testGetLongWithoutDefaultThrowsOnEmptyString() {
228+
// Test getLong without default value when property is empty string
229+
Properties properties = new Properties();
230+
properties.put("empty.long", "");
231+
properties.put("whitespace.long", " ");
232+
VeniceProperties veniceProperties = new VeniceProperties(properties);
233+
234+
// Empty string should throw VeniceException
235+
VeniceException ex1 = expectThrows(VeniceException.class, () -> veniceProperties.getLong("empty.long"));
236+
Assert.assertTrue(ex1.getMessage().contains("empty value"));
237+
238+
// Whitespace-only string should throw VeniceException
239+
VeniceException ex2 = expectThrows(VeniceException.class, () -> veniceProperties.getLong("whitespace.long"));
240+
Assert.assertTrue(ex2.getMessage().contains("empty value"));
241+
242+
// Missing property should throw UndefinedPropertyException
243+
expectThrows(UndefinedPropertyException.class, () -> veniceProperties.getLong("missing.long"));
244+
}
245+
246+
@Test
247+
public void testGetIntWithEmptyString() {
248+
// Test getInt with default value when property is empty string
249+
Properties properties = new Properties();
250+
properties.put("empty.int", "");
251+
properties.put("whitespace.int", " ");
252+
properties.put("valid.int", "42");
253+
VeniceProperties veniceProperties = new VeniceProperties(properties);
254+
255+
// Empty string should return default value
256+
assertEquals(veniceProperties.getInt("empty.int", 100), 100);
257+
258+
// Whitespace-only string should return default value
259+
assertEquals(veniceProperties.getInt("whitespace.int", 200), 200);
260+
261+
// Valid value should be parsed
262+
assertEquals(veniceProperties.getInt("valid.int", 100), 42);
263+
264+
// Missing property should return default value
265+
assertEquals(veniceProperties.getInt("missing.int", 300), 300);
266+
}
267+
268+
@Test
269+
public void testGetIntWithoutDefaultThrowsOnEmptyString() {
270+
// Test getInt without default value when property is empty string
271+
Properties properties = new Properties();
272+
properties.put("empty.int", "");
273+
properties.put("whitespace.int", " ");
274+
VeniceProperties veniceProperties = new VeniceProperties(properties);
275+
276+
// Empty string should throw VeniceException
277+
VeniceException ex1 = expectThrows(VeniceException.class, () -> veniceProperties.getInt("empty.int"));
278+
Assert.assertTrue(ex1.getMessage().contains("empty value"));
279+
280+
// Whitespace-only string should throw VeniceException
281+
VeniceException ex2 = expectThrows(VeniceException.class, () -> veniceProperties.getInt("whitespace.int"));
282+
Assert.assertTrue(ex2.getMessage().contains("empty value"));
283+
284+
// Missing property should throw UndefinedPropertyException
285+
expectThrows(UndefinedPropertyException.class, () -> veniceProperties.getInt("missing.int"));
286+
}
287+
288+
@Test
289+
public void testGetOptionalIntWithEmptyString() {
290+
// Test getOptionalInt when property is empty string
291+
Properties properties = new Properties();
292+
properties.put("empty.int", "");
293+
properties.put("whitespace.int", " ");
294+
properties.put("valid.int", "123");
295+
VeniceProperties veniceProperties = new VeniceProperties(properties);
296+
297+
// Empty string should return Optional.empty()
298+
Assert.assertFalse(veniceProperties.getOptionalInt("empty.int").isPresent());
299+
300+
// Whitespace-only string should return Optional.empty()
301+
Assert.assertFalse(veniceProperties.getOptionalInt("whitespace.int").isPresent());
302+
303+
// Valid value should be present
304+
Assert.assertTrue(veniceProperties.getOptionalInt("valid.int").isPresent());
305+
assertEquals(veniceProperties.getOptionalInt("valid.int").get(), Integer.valueOf(123));
306+
307+
// Missing property should return Optional.empty()
308+
Assert.assertFalse(veniceProperties.getOptionalInt("missing.int").isPresent());
309+
}
310+
311+
@Test
312+
public void testGetDoubleWithEmptyString() {
313+
// Test getDouble with default value when property is empty string
314+
Properties properties = new Properties();
315+
properties.put("empty.double", "");
316+
properties.put("whitespace.double", " ");
317+
properties.put("valid.double", "3.14");
318+
VeniceProperties veniceProperties = new VeniceProperties(properties);
319+
320+
// Empty string should return default value
321+
assertEquals(veniceProperties.getDouble("empty.double", 1.5), 1.5);
322+
323+
// Whitespace-only string should return default value
324+
assertEquals(veniceProperties.getDouble("whitespace.double", 2.5), 2.5);
325+
326+
// Valid value should be parsed
327+
assertEquals(veniceProperties.getDouble("valid.double", 1.5), 3.14);
328+
329+
// Missing property should return default value
330+
assertEquals(veniceProperties.getDouble("missing.double", 4.5), 4.5);
331+
}
332+
333+
@Test
334+
public void testGetDoubleWithoutDefaultThrowsOnEmptyString() {
335+
// Test getDouble without default value when property is empty string
336+
Properties properties = new Properties();
337+
properties.put("empty.double", "");
338+
properties.put("whitespace.double", " ");
339+
VeniceProperties veniceProperties = new VeniceProperties(properties);
340+
341+
// Empty string should throw VeniceException
342+
VeniceException ex1 = expectThrows(VeniceException.class, () -> veniceProperties.getDouble("empty.double"));
343+
Assert.assertTrue(ex1.getMessage().contains("empty value"));
344+
345+
// Whitespace-only string should throw VeniceException
346+
VeniceException ex2 = expectThrows(VeniceException.class, () -> veniceProperties.getDouble("whitespace.double"));
347+
Assert.assertTrue(ex2.getMessage().contains("empty value"));
348+
349+
// Missing property should throw UndefinedPropertyException
350+
expectThrows(UndefinedPropertyException.class, () -> veniceProperties.getDouble("missing.double"));
351+
}
352+
353+
@Test
354+
public void testGetSizeInBytesWithEmptyString() {
355+
// Test getSizeInBytes with default value when property is empty string
356+
Properties properties = new Properties();
357+
properties.put("empty.size", "");
358+
properties.put("whitespace.size", " ");
359+
properties.put("valid.size", "10MB");
360+
VeniceProperties veniceProperties = new VeniceProperties(properties);
361+
362+
// Empty string should return default value
363+
assertEquals(veniceProperties.getSizeInBytes("empty.size", 1024L), 1024L);
364+
365+
// Whitespace-only string should return default value
366+
assertEquals(veniceProperties.getSizeInBytes("whitespace.size", 2048L), 2048L);
367+
368+
// Valid value should be parsed
369+
assertEquals(veniceProperties.getSizeInBytes("valid.size", 1024L), 10 * 1024 * 1024L);
370+
371+
// Missing property should return default value
372+
assertEquals(veniceProperties.getSizeInBytes("missing.size", 4096L), 4096L);
373+
}
374+
375+
@Test
376+
public void testGetSizeInBytesWithoutDefaultThrowsOnEmptyString() {
377+
// Test getSizeInBytes without default value when property is empty string
378+
Properties properties = new Properties();
379+
properties.put("empty.size", "");
380+
properties.put("whitespace.size", " ");
381+
VeniceProperties veniceProperties = new VeniceProperties(properties);
382+
383+
// Empty string should throw VeniceException
384+
VeniceException ex1 = expectThrows(VeniceException.class, () -> veniceProperties.getSizeInBytes("empty.size"));
385+
Assert.assertTrue(ex1.getMessage().contains("empty value"));
386+
387+
// Whitespace-only string should throw VeniceException
388+
VeniceException ex2 = expectThrows(VeniceException.class, () -> veniceProperties.getSizeInBytes("whitespace.size"));
389+
Assert.assertTrue(ex2.getMessage().contains("empty value"));
390+
391+
// Missing property should throw UndefinedPropertyException
392+
expectThrows(UndefinedPropertyException.class, () -> veniceProperties.getSizeInBytes("missing.size"));
393+
}
203394
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ public String registerTasks(String clientId, StoreChangeTasks tasks) {
126126
String uniqueTaskId = generateUniqueTaskId(clientId);
127127
taskRegistry.put(uniqueTaskId, tasks);
128128

129-
LOGGER.info("Registered tasks with ID: {} (client: {})", uniqueTaskId, clientId);
129+
LOGGER.debug("Registered tasks with ID: {} (client: {})", uniqueTaskId, clientId);
130130
return uniqueTaskId;
131131
}
132132

@@ -399,7 +399,7 @@ private void submitTask(String taskId, Runnable taskRunnable, StoreChangeEventTy
399399
try {
400400
notificationExecutor.submit(() -> {
401401
try {
402-
LOGGER.info("Executing task {} for {} event: {}", taskId, eventType.name(), context);
402+
LOGGER.debug("Executing task {} for {} event: {}", taskId, eventType.name(), context);
403403
taskRunnable.run();
404404
} catch (Exception e) {
405405
// Catch all exceptions to prevent one task failure from affecting others

0 commit comments

Comments
 (0)