Skip to content

SAMZA-2318: Empty config values from coordinator stream shouldn't be removed #1150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cameronlee314 merged 3 commits into from
Sep 13, 2019
Merged

SAMZA-2318: Empty config values from coordinator stream shouldn't be removed #1150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a833a3c
SAMZA-2318: Empty config values from coordinator stream shouldn't be …
Sep 11, 2019
5efb6fe
add unit-test
Sep 12, 2019
ec43122
Add null value to config map for testing
Sep 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions samza-core/src/main/scala/org/apache/samza/util/CoordinatorStreamUtil.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,8 @@ object CoordinatorStreamUtil extends Logging {
} else {
val valueSerde: CoordinatorStreamValueSerde = new CoordinatorStreamValueSerde(SetConfig.TYPE)
val valueAsString: String = valueSerde.fromBytes(valueAsBytes)
if (StringUtils.isBlank(valueAsString)) {
warn("Value for key: %s in config is empty or null. Ignoring it." format key)
if (valueAsString == null) {
warn("Value for key: %s in config is decoded to be null. Ignoring it." format key)

@dnishimura dnishimura Sep 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be useful to add a unit test to test that null values appear as non-entries and empty string values are entries with an empty string when loaded from the coordinator stream.

@dengpanyin dengpanyin Sep 12, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dnishimura UNIT-Test added Thanks

} else {
configMap.put(key, valueAsString)
}
Expand Down
36 changes: 35 additions & 1 deletion samza-core/src/test/scala/org/apache/samza/util/TestCoordinatorStreamUtil.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,13 @@
*/
package org.apache.samza.util

import java.util

import org.apache.samza.coordinator.metadatastore.CoordinatorStreamStore
import org.apache.samza.coordinator.stream.CoordinatorStreamValueSerde
import org.apache.samza.coordinator.stream.messages.SetConfig
import org.apache.samza.system.{StreamSpec, SystemAdmin, SystemStream}
import org.junit.Test
import org.junit.{Assert, Test}
import org.mockito.Matchers.any
import org.mockito.Mockito

Expand All @@ -34,4 +39,33 @@ class TestCoordinatorStreamUtil {
Mockito.verify(systemStream).getStream
Mockito.verify(systemAdmin).createStream(any(classOf[StreamSpec]))
}

@Test
def testReadConfigFromCoordinatorStream {
val keyForNonBlankVal = "app.id"
val nonBlankVal = "1"
val keyForEmptyVal = "task.opt"
val emptyVal = ""
val keyForNullVal = "zk.server"
val nullVal = null
Comment thread
dengpanyin marked this conversation as resolved.

val valueSerde = new CoordinatorStreamValueSerde(SetConfig.TYPE)
val configMap = new util.HashMap[String, Array[Byte]]() {
put(CoordinatorStreamStore.serializeCoordinatorMessageKeyToJson(SetConfig.TYPE, keyForNonBlankVal),
valueSerde.toBytes(nonBlankVal))
put(CoordinatorStreamStore.serializeCoordinatorMessageKeyToJson(SetConfig.TYPE, keyForEmptyVal),
valueSerde.toBytes(emptyVal))
put(CoordinatorStreamStore.serializeCoordinatorMessageKeyToJson(SetConfig.TYPE, keyForNullVal),
valueSerde.toBytes(nullVal))
}

val coordinatorStreamStore = Mockito.mock(classOf[CoordinatorStreamStore])
Mockito.when(coordinatorStreamStore.all()).thenReturn(configMap)

val configFromCoordinatorStream = CoordinatorStreamUtil.readConfigFromCoordinatorStream(coordinatorStreamStore)

Assert.assertEquals(configFromCoordinatorStream.get(keyForNonBlankVal), nonBlankVal)
Assert.assertEquals(configFromCoordinatorStream.get(keyForEmptyVal), emptyVal)
Assert.assertFalse(configFromCoordinatorStream.containsKey(keyForNullVal))
}
}