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 2022/09/19 22:28:15 UTC
[GitHub] [samza] jia-gao opened a new pull request, #1631: emit to diagnostic kafka topic in round robin fashion to support increasing this topic partition count
jia-gao opened a new pull request, #1631:
URL: https://github.com/apache/samza/pull/1631
Issue:
Currently, diagnostics topics for samza jobs are created with a single partition.
for some jobs the size of the this partition can grow to be large. thus there could be a need to increase partition count of the topic.
however, DiagnosticsManager within samza-li framework uses hostname of the container as the partition key while emiting the msg to kafka. Due to hostnames at LinkedIn being very similar, the hash of these in partition key will not evenly distribute the msgs across partitions.
Change: thus there needs to be work done to emit in round robin fashion. this can be achieved by keeping partition key as null.
Test Done:
./gradlew build
--
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.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] lakshmi-manasa-g commented on pull request #1631: emit to diagnostic kafka topic in round robin fashion to support increasing this topic partition count
Posted by GitBox <gi...@apache.org>.
lakshmi-manasa-g commented on PR #1631:
URL: https://github.com/apache/samza/pull/1631#issuecomment-1258231304
> In terms of the assumption on diagnostics stream is stream with one partition count, can that be created explicitly by the user? Or potentially expanded to more than one partition?
>In that case, it could still mean a incompatible change right?
the default creation for diag stream inside samza framework code is with 1 partition. that said, i think it can be expanded to more than 1 partition. Maybe if the stream is pre-created before the job creates it then it can be created with >1 as well.
Yes, if the diag stream has >1 partition prior to upgrading to this PR, then it could be an incompatible change.
thanks for catching this.
@jia-gao can you please mark this PR as backwards incompatible along with an explanation as to WHEN it is backwards incompatible. without the when part, this might be seen as a breaking change in all cases which it is not.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat commented on pull request #1631: emit to diagnostic kafka topic in round robin fashion to support increasing this topic partition count
Posted by GitBox <gi...@apache.org>.
mynameborat commented on PR #1631:
URL: https://github.com/apache/samza/pull/1631#issuecomment-1255408139
>
Just noticed the constructor this is the partition key and not the message key. It should be okay from that front. In terms of the assumption on diagnostics stream is stream with one partition count, can that be created explicitly by the user? Or potentially expanded to more than one partition?
In that case, it could still mean a incompatible change right?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] lakshmi-manasa-g commented on pull request #1631: emit to diagnostic kafka topic in round robin fashion to support increasing this topic partition count
Posted by GitBox <gi...@apache.org>.
lakshmi-manasa-g commented on PR #1631:
URL: https://github.com/apache/samza/pull/1631#issuecomment-1254380649
> Is the partition key field not used as part of the emitted payload in downstream? That does seem like a backward incompatible change given the messages produced going forward wouldn't have the hostname exposed as key of the message?
partition key field is used to determine which partition of the diagnostics stream to send the message to right. partition key is not part of the payload itself the payload for these messages is MetricsSnapshotSerdeV2().toBytes(diagnosticsStreamMessage.convertToMetricsSnapshot.
the most commonly used diagnostics system aka kafka for uses partition key for routing the msg key (which is null here) and msg value (which is the metrics snapshot) pair to the appropriate partition. partition key does not become the msg key right, msg key is explicitly set to null here.
given that diagnostic stream is created with 1 partition count by default, this change will be backwards compatible right.
did you mean the case where partition count is set to larger than 1 then this is not backwards compatible?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat merged pull request #1631: emit to diagnostic kafka topic in round robin fashion to support increasing this topic partition count
Posted by GitBox <gi...@apache.org>.
mynameborat merged PR #1631:
URL: https://github.com/apache/samza/pull/1631
--
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.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] jia-gao commented on pull request #1631: emit to diagnostic kafka topic in round robin fashion to support increasing this topic partition count
Posted by GitBox <gi...@apache.org>.
jia-gao commented on PR #1631:
URL: https://github.com/apache/samza/pull/1631#issuecomment-1252600397
> lets please follow https://cwiki.apache.org/confluence/display/SAMZA/SEP-25%3A+PR+Title+And+Description+Guidelines for all samza PR descriptions
>
> We also need to explain that for open source samza, the partition count is 1 for diagnostics stream due to DiagnosticsUtil.createDiagnosticsStream which relies on default parttition count within stream spec which is set to 1. hence this change will not break anything for open source community and is backwards compatible. However, it allows for better distrubution if the diag topic's partition count is increased and some info on how null partition key helps for the most commonly used system aka kafka.
Thanks, updated
--
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.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] jia-gao commented on pull request #1631: emit to diagnostic kafka topic in round robin fashion to support increasing this topic partition count
Posted by GitBox <gi...@apache.org>.
jia-gao commented on PR #1631:
URL: https://github.com/apache/samza/pull/1631#issuecomment-1260146954
> > In terms of the assumption on diagnostics stream is stream with one partition count, can that be created explicitly by the user? Or potentially expanded to more than one partition?
> > In that case, it could still mean a incompatible change right?
>
> the default creation for diag stream inside samza framework code is with 1 partition. that said, i think it can be expanded to more than 1 partition. Maybe if the stream is pre-created before the job creates it then it can be created with >1 as well. Yes, if the diag stream has >1 partition prior to upgrading to this PR, then it could be an incompatible change. thanks for catching this.
>
> @jia-gao can you please mark this PR as backwards incompatible along with an explanation as to WHEN it is backwards incompatible. without the when part, this might be seen as a breaking change in all cases which it is not.
make sense, updated the description
--
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.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat commented on pull request #1631: emit to diagnostic kafka topic in round robin fashion to support increasing this topic partition count
Posted by GitBox <gi...@apache.org>.
mynameborat commented on PR #1631:
URL: https://github.com/apache/samza/pull/1631#issuecomment-1254315368
Is the partition key field not used as part of the emitted payload in downstream? That does seem like a backward incompatible change given the messages produced going forward wouldn't have the hostname exposed as key of the message?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] lakshmi-manasa-g commented on pull request #1631: emit to diagnostic kafka topic in round robin fashion to support increasing this topic partition count
Posted by GitBox <gi...@apache.org>.
lakshmi-manasa-g commented on PR #1631:
URL: https://github.com/apache/samza/pull/1631#issuecomment-1252499872
lets please follow https://cwiki.apache.org/confluence/display/SAMZA/SEP-25%3A+PR+Title+And+Description+Guidelines for all samza PR descriptions
We also need to explain that for open source samza, the partition count is 1 for diagnostics stream due to DiagnosticsUtil.createDiagnosticsStream which relies on default parttition count within stream spec which is set to 1.
hence this change will not break anything for open source community and is backwards compatible. However, it allows for better distrubution if the diag topic's partition count is increased and some info on how null partition key helps for the most commonly used system aka kafka.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org