You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by GitBox <gi...@apache.org> on 2021/03/03 01:18:53 UTC
[GitHub] [samza] Sanil15 opened a new pull request #1468: SAMZA-2626: NoOp serde configured for a local table or a storage engine based table is skipped while generating store configs
Sanil15 opened a new pull request #1468:
URL: https://github.com/apache/samza/pull/1468
**Issues:** JobNodeConfigurationGenerator assumes that a Table based on a StorageEngine API cannot have a NoOpSerde, and while generating store configs for tables NoOpSerde is treated equivalent to null (not present cast) for both key & msg serdes
**Changes:** Fixed the JobNodeConfigurationGenerator to not ignore a NoOpSerde configuration for store-based of local tables
**Tests:** Added a unit test
**API Changes:** None
**Upgrade Instructions:** None
**Usage Instructions:** None
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] Sanil15 closed pull request #1468: SAMZA-2626: NoOp serde configured for a local table or a storage engine based table is skipped while generating store configs
Posted by GitBox <gi...@apache.org>.
Sanil15 closed pull request #1468:
URL: https://github.com/apache/samza/pull/1468
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] Sanil15 commented on pull request #1468: SAMZA-2626: NoOp serde configured for a local table or a storage engine based table is skipped while generating store configs
Posted by GitBox <gi...@apache.org>.
Sanil15 commented on pull request #1468:
URL: https://github.com/apache/samza/pull/1468#issuecomment-790915529
> Can you clarify the motivation on why you need to differentiate `NoOpSerde` and `null` within container? Please update the description so that its useful for later references.
Done added more description on why do I need a support for NoOp serde
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat commented on a change in pull request #1468: SAMZA-2626: NoOp serde configured for a local table or a storage engine based table is skipped while generating store configs
Posted by GitBox <gi...@apache.org>.
mynameborat commented on a change in pull request #1468:
URL: https://github.com/apache/samza/pull/1468#discussion_r587921386
##########
File path: samza-core/src/main/java/org/apache/samza/execution/JobNodeConfigurationGenerator.java
##########
@@ -351,10 +351,10 @@ private void configureSerdes(Map<String, String> configs, Map<String, StreamEdge
private void addSerdes(KV<Serde, Serde> serdes, String streamId, Map<String, Serde> keySerdeMap,
Map<String, Serde> msgSerdeMap) {
if (serdes != null) {
- if (serdes.getKey() != null && !(serdes.getKey() instanceof NoOpSerde)) {
+ if (serdes.getKey() != null) {
keySerdeMap.put(streamId, serdes.getKey());
}
- if (serdes.getValue() != null && !(serdes.getValue() instanceof NoOpSerde)) {
+ if (serdes.getValue() != null) {
Review comment:
what class description are you referring to?
Like I pointed out earlier, the harm is creating `copies of serialized serdes & corresponding configurations`. I couldn't find much of historical context here.
@prateekm or @nickpan47 do you guys know why we specifically added this check for `instanceof NoOpSerde` and exclude them?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat commented on a change in pull request #1468: SAMZA-2626: NoOp serde configured for a local table or a storage engine based table is skipped while generating store configs
Posted by GitBox <gi...@apache.org>.
mynameborat commented on a change in pull request #1468:
URL: https://github.com/apache/samza/pull/1468#discussion_r586148791
##########
File path: samza-core/src/main/java/org/apache/samza/execution/JobNodeConfigurationGenerator.java
##########
@@ -351,10 +351,10 @@ private void configureSerdes(Map<String, String> configs, Map<String, StreamEdge
private void addSerdes(KV<Serde, Serde> serdes, String streamId, Map<String, Serde> keySerdeMap,
Map<String, Serde> msgSerdeMap) {
if (serdes != null) {
- if (serdes.getKey() != null && !(serdes.getKey() instanceof NoOpSerde)) {
+ if (serdes.getKey() != null) {
keySerdeMap.put(streamId, serdes.getKey());
}
- if (serdes.getValue() != null && !(serdes.getValue() instanceof NoOpSerde)) {
+ if (serdes.getValue() != null) {
Review comment:
This will impact the maps being returned to the caller which can now have multiple instances of no-op serde even though in theory all of them are logically the same.
While this may not impact functionality, it does create redundant copies of serialized serdes & configurations.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] prateekm commented on pull request #1468: SAMZA-2626: NoOp serde configured for a local table or a storage engine based table is skipped while generating store configs
Posted by GitBox <gi...@apache.org>.
prateekm commented on pull request #1468:
URL: https://github.com/apache/samza/pull/1468#issuecomment-791647260
@Sanil15 NoOpSerde is not a real serde (check its implementation). It's a marker to indicate to framework that a "no op / pass through" serde should be used instead. What that means depends on the context it's being used in. How will passing the NoOpSerde to the BaseKVStorageEngineFactory help here?
Do you instead need to modify ContainerStorageManager & BaseKVStorageEngineFactory etc to allow passing null as the key or msg serde?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] Sanil15 commented on pull request #1468: SAMZA-2626: NoOp serde configured for a local table or a storage engine based table is skipped while generating store configs
Posted by GitBox <gi...@apache.org>.
Sanil15 commented on pull request #1468:
URL: https://github.com/apache/samza/pull/1468#issuecomment-797781776
https://github.com/apache/samza/pull/1475 Pr fixes this correctly
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] Sanil15 commented on a change in pull request #1468: SAMZA-2626: NoOp serde configured for a local table or a storage engine based table is skipped while generating store configs
Posted by GitBox <gi...@apache.org>.
Sanil15 commented on a change in pull request #1468:
URL: https://github.com/apache/samza/pull/1468#discussion_r587799551
##########
File path: samza-core/src/main/java/org/apache/samza/execution/JobNodeConfigurationGenerator.java
##########
@@ -351,10 +351,10 @@ private void configureSerdes(Map<String, String> configs, Map<String, StreamEdge
private void addSerdes(KV<Serde, Serde> serdes, String streamId, Map<String, Serde> keySerdeMap,
Map<String, Serde> msgSerdeMap) {
if (serdes != null) {
- if (serdes.getKey() != null && !(serdes.getKey() instanceof NoOpSerde)) {
+ if (serdes.getKey() != null) {
keySerdeMap.put(streamId, serdes.getKey());
}
- if (serdes.getValue() != null && !(serdes.getValue() instanceof NoOpSerde)) {
+ if (serdes.getValue() != null) {
Review comment:
The class description suggests we do not want to share references of serdes but even if this creates a NoOp serve copy for each stream, store, table how can it hurt?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] Sanil15 commented on a change in pull request #1468: SAMZA-2626: NoOp serde configured for a local table or a storage engine based table is skipped while generating store configs
Posted by GitBox <gi...@apache.org>.
Sanil15 commented on a change in pull request #1468:
URL: https://github.com/apache/samza/pull/1468#discussion_r587949755
##########
File path: samza-core/src/main/java/org/apache/samza/execution/JobNodeConfigurationGenerator.java
##########
@@ -351,10 +351,10 @@ private void configureSerdes(Map<String, String> configs, Map<String, StreamEdge
private void addSerdes(KV<Serde, Serde> serdes, String streamId, Map<String, Serde> keySerdeMap,
Map<String, Serde> msgSerdeMap) {
if (serdes != null) {
- if (serdes.getKey() != null && !(serdes.getKey() instanceof NoOpSerde)) {
+ if (serdes.getKey() != null) {
keySerdeMap.put(streamId, serdes.getKey());
}
- if (serdes.getValue() != null && !(serdes.getValue() instanceof NoOpSerde)) {
+ if (serdes.getValue() != null) {
Review comment:
@bharathkk sorry i meant java docs on line 265
```
* We try to preserve the number of Serde instances before and after serialization. However we don't
* guarantee that references shared between these serdes instances (e.g. an Jackson ObjectMapper shared
* between two json serdes) are shared after deserialization too.
*
* Ideally all the user defined objects in the application should be serialized and de-serialized in one pass
* from the same output/input stream so that we can maintain reference sharing relationships.
*
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] Sanil15 commented on pull request #1468: SAMZA-2626: NoOp serde configured for a local table or a storage engine based table is skipped while generating store configs
Posted by GitBox <gi...@apache.org>.
Sanil15 commented on pull request #1468:
URL: https://github.com/apache/samza/pull/1468#issuecomment-797678963
Discarding this PR because of incorrect assumptions will follow up with another one to fix this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org