You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by pasalkarsachin1 <gi...@git.apache.org> on 2017/02/14 10:28:03 UTC
[GitHub] storm pull request #1939: STORM-1363: TridentKafkaState should handle null v...
GitHub user pasalkarsachin1 opened a pull request:
https://github.com/apache/storm/pull/1939
STORM-1363: TridentKafkaState should handle null values from TridentTupleToKafkaMapper.getMessageFromTuple()
In case null value comes from the mapper it will print warning messages.
Added log to print the time taken to emit number of messages.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pasalkarsachin1/storm STORM-1363
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1939.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1939
----
commit f78760c7ed94b14312d0eae1c7b5688c7eb4e96d
Author: Sachin Pasalkar <sa...@symantec.com>
Date: 2017-02-14T10:24:23Z
STORM-1363: TridentKafkaState should handle null values from TridentTupleToKafkaMapper.getMessageFromTuple()
Incase null value comes from the mapper it will print warning messages also added the time take to emit number od messages in logs
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1939: STORM-1363: TridentKafkaState should handle null values f...
Posted by pasalkarsachin1 <gi...@git.apache.org>.
Github user pasalkarsachin1 commented on the issue:
https://github.com/apache/storm/pull/1939
@HeartSaVioR Can you merge this so I will close this PR :) Thanks in advance
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1939: STORM-1363: TridentKafkaState should handle null v...
Posted by pasalkarsachin1 <gi...@git.apache.org>.
Github user pasalkarsachin1 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1939#discussion_r101993619
--- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/trident/TridentKafkaState.java ---
@@ -98,14 +107,19 @@ public void updateState(List<TridentTuple> tuples, TridentCollector collector) {
}
if(exceptions.size() > 0){
- String errorMsg = "Could not retrieve result for messages " + tuples + " from topic = " + topic
- + " because of the following exceptions: \n";
- for (ExecutionException exception : exceptions) {
- errorMsg = errorMsg + exception.getMessage() + "\n";
- }
- LOG.error(errorMsg);
- throw new FailedException(errorMsg);
- }
+ StringBuilder errorMsg = new StringBuilder("Could not retrieve result for messages ").append(tuples)
+ .append(" from topic = ").append(topic).append(" because of the following exceptions: ").append(System.lineSeparator());
+
+ for (ExecutionException exception : exceptions) {
+ errorMsg = errorMsg.append(exception.getMessage()).append(System.lineSeparator()); ;
+ }
+ String message = errorMsg.toString();
+ LOG.error(message);
+ throw new FailedException(message);
+ }
+ long latestTime=System.currentTimeMillis();
+ LOG.info("Emitted record {} sucessfully in {} ms to topic {} ", new Object[]{emittedRecords,latestTime-currentTime,topic});
--- End diff --
In our production currently there is no way you get information of how many records are getting pushed from logs. This log has turned out to provide at least so information on records pushed to Kafka in what time. This also helps who took time whether Kafka was slow in responding or we have large chunk of messages sent to Kafka
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1939: STORM-1363: TridentKafkaState should handle null v...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1939#discussion_r101977245
--- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/trident/TridentKafkaState.java ---
@@ -75,20 +75,29 @@ public void prepare(Properties options) {
public void updateState(List<TridentTuple> tuples, TridentCollector collector) {
String topic = null;
try {
- List<Future<RecordMetadata>> futures = new ArrayList<>(tuples.size());
+ long currentTime=System.currentTimeMillis();
+ int numberOfRecords = tuples.size();
+ List<Future<RecordMetadata>> futures = new ArrayList<>(numberOfRecords);
for (TridentTuple tuple : tuples) {
topic = topicSelector.getTopic(tuple);
-
+ Object messageFromTuple = mapper.getMessageFromTuple(tuple);
+ Object keyFromTuple = mapper.getKeyFromTuple(tuple);
+
if(topic != null) {
- Future<RecordMetadata> result = producer.send(new ProducerRecord(topic,
- mapper.getKeyFromTuple(tuple), mapper.getMessageFromTuple(tuple)));
- futures.add(result);
+ if(messageFromTuple!=null){
+ Future<RecordMetadata> result = producer.send(new ProducerRecord(topic,keyFromTuple, messageFromTuple));
+ futures.add(result);
+ } else {
+ LOG.warn("skipping Message with Key "+keyFromTuple+" as message was null");
--- End diff --
spaces between `"` and `+` / and `+` and `"`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1939: STORM-1363: TridentKafkaState should handle null v...
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on a diff in the pull request:
https://github.com/apache/storm/pull/1939#discussion_r101950396
--- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/trident/TridentKafkaState.java ---
@@ -98,14 +107,19 @@ public void updateState(List<TridentTuple> tuples, TridentCollector collector) {
}
if(exceptions.size() > 0){
- String errorMsg = "Could not retrieve result for messages " + tuples + " from topic = " + topic
- + " because of the following exceptions: \n";
- for (ExecutionException exception : exceptions) {
- errorMsg = errorMsg + exception.getMessage() + "\n";
- }
- LOG.error(errorMsg);
- throw new FailedException(errorMsg);
- }
+ StringBuilder errorMsg = new StringBuilder("Could not retrieve result for messages ").append(tuples)
--- End diff --
Java auto converts string concats to StringBuilder. There is no need for this. See discussion here for when is useful to use StringBuilder
http://stackoverflow.com/questions/11942368/why-use-stringbuilder-explicitly-if-the-compiler-converts-string-concatenation-t
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1939: STORM-1363: TridentKafkaState should handle null v...
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on a diff in the pull request:
https://github.com/apache/storm/pull/1939#discussion_r101951981
--- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/trident/TridentKafkaState.java ---
@@ -98,14 +107,19 @@ public void updateState(List<TridentTuple> tuples, TridentCollector collector) {
}
if(exceptions.size() > 0){
- String errorMsg = "Could not retrieve result for messages " + tuples + " from topic = " + topic
- + " because of the following exceptions: \n";
- for (ExecutionException exception : exceptions) {
- errorMsg = errorMsg + exception.getMessage() + "\n";
- }
- LOG.error(errorMsg);
- throw new FailedException(errorMsg);
- }
+ StringBuilder errorMsg = new StringBuilder("Could not retrieve result for messages ").append(tuples)
--- End diff --
makes sense.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1939: STORM-1363: TridentKafkaState should handle null v...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1939#discussion_r101977581
--- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/trident/TridentKafkaState.java ---
@@ -75,20 +75,29 @@ public void prepare(Properties options) {
public void updateState(List<TridentTuple> tuples, TridentCollector collector) {
String topic = null;
try {
- List<Future<RecordMetadata>> futures = new ArrayList<>(tuples.size());
+ long currentTime=System.currentTimeMillis();
--- End diff --
1. `startTime` would be better to represent its intention well
2. space between `e` and `=` and `=` and `S`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1939: STORM-1363: TridentKafkaState should handle null values f...
Posted by pasalkarsachin1 <gi...@git.apache.org>.
Github user pasalkarsachin1 commented on the issue:
https://github.com/apache/storm/pull/1939
Can someone take a look at this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1939: STORM-1363: TridentKafkaState should handle null values f...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1939
@pasalkarsachin1
We don't use 8 spaces for indentation from other java files. If the indentation spaces was 4, it should be 4 unless we set brand new style guide. Code style is important as much as code change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1939: STORM-1363: TridentKafkaState should handle null v...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1939#discussion_r101974017
--- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/trident/TridentKafkaState.java ---
@@ -98,14 +107,19 @@ public void updateState(List<TridentTuple> tuples, TridentCollector collector) {
}
if(exceptions.size() > 0){
- String errorMsg = "Could not retrieve result for messages " + tuples + " from topic = " + topic
- + " because of the following exceptions: \n";
- for (ExecutionException exception : exceptions) {
- errorMsg = errorMsg + exception.getMessage() + "\n";
- }
- LOG.error(errorMsg);
- throw new FailedException(errorMsg);
- }
+ StringBuilder errorMsg = new StringBuilder("Could not retrieve result for messages ").append(tuples)
+ .append(" from topic = ").append(topic).append(" because of the following exceptions: ").append(System.lineSeparator());
+
+ for (ExecutionException exception : exceptions) {
+ errorMsg = errorMsg.append(exception.getMessage()).append(System.lineSeparator()); ;
+ }
+ String message = errorMsg.toString();
+ LOG.error(message);
+ throw new FailedException(message);
+ }
+ long latestTime=System.currentTimeMillis();
+ LOG.info("Emitted record {} sucessfully in {} ms to topic {} ", new Object[]{emittedRecords,latestTime-currentTime,topic});
--- End diff --
I think it receives varargs, so no necessary to group into `Object []` explicitly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1939: STORM-1363: TridentKafkaState should handle null v...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1939#discussion_r101976272
--- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/trident/TridentKafkaState.java ---
@@ -98,14 +107,19 @@ public void updateState(List<TridentTuple> tuples, TridentCollector collector) {
}
if(exceptions.size() > 0){
- String errorMsg = "Could not retrieve result for messages " + tuples + " from topic = " + topic
- + " because of the following exceptions: \n";
- for (ExecutionException exception : exceptions) {
- errorMsg = errorMsg + exception.getMessage() + "\n";
- }
- LOG.error(errorMsg);
- throw new FailedException(errorMsg);
- }
+ StringBuilder errorMsg = new StringBuilder("Could not retrieve result for messages ").append(tuples)
--- End diff --
Personally I'd rather using StringBuilder to only concat exception messages. For me current implementation is better for readability, and compiler is able to care of this.
(Please note that those lines are not - and should not be - hot spot, since code block will be executed only when there're some exceptions.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1939: STORM-1363: TridentKafkaState should handle null values f...
Posted by pasalkarsachin1 <gi...@git.apache.org>.
Github user pasalkarsachin1 commented on the issue:
https://github.com/apache/storm/pull/1939
@HeartSaVioR Squashed into one commit. Commit sha is 7cc1bdbea562a48aa09cc6dc7fd29edd5bd4eb10
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1939: STORM-1363: TridentKafkaState should handle null values f...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1939
@pasalkarsachin1 Could you preserve the style? I'm sorry we don't have clear style guide but most of changeset is from spaces.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1939: STORM-1363: TridentKafkaState should handle null values f...
Posted by pasalkarsachin1 <gi...@git.apache.org>.
Github user pasalkarsachin1 commented on the issue:
https://github.com/apache/storm/pull/1939
@HeartSaVioR I have updated code as per your comment :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1939: STORM-1363: TridentKafkaState should handle null values f...
Posted by pasalkarsachin1 <gi...@git.apache.org>.
Github user pasalkarsachin1 commented on the issue:
https://github.com/apache/storm/pull/1939
@HeartSaVioR I did my best. Can you take a look at it now
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1939: STORM-1363: TridentKafkaState should handle null values f...
Posted by pasalkarsachin1 <gi...@git.apache.org>.
Github user pasalkarsachin1 commented on the issue:
https://github.com/apache/storm/pull/1939
@HeartSaVioR Can you take a look at it
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1939: STORM-1363: TridentKafkaState should handle null v...
Posted by pasalkarsachin1 <gi...@git.apache.org>.
Github user pasalkarsachin1 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1939#discussion_r101951192
--- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/trident/TridentKafkaState.java ---
@@ -98,14 +107,19 @@ public void updateState(List<TridentTuple> tuples, TridentCollector collector) {
}
if(exceptions.size() > 0){
- String errorMsg = "Could not retrieve result for messages " + tuples + " from topic = " + topic
- + " because of the following exceptions: \n";
- for (ExecutionException exception : exceptions) {
- errorMsg = errorMsg + exception.getMessage() + "\n";
- }
- LOG.error(errorMsg);
- throw new FailedException(errorMsg);
- }
+ StringBuilder errorMsg = new StringBuilder("Could not retrieve result for messages ").append(tuples)
--- End diff --
If you read statement in stack overflow
"However StringBuilder is useful if you are concatenating a large number of strings in a loop. It requires O(n2) time to run and creates many temporary strings."
We also have same case.
```java
for (ExecutionException exception : exceptions) {
errorMsg = errorMsg.append(exception.getMessage()).append(System.lineSeparator()); ;
}
```
So rather than having String above loop & then recreate StringBuilder object & created it initially.
StringBuilder object
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1939: STORM-1363: TridentKafkaState should handle null values f...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1939
@pasalkarsachin1
Indentation is still off. Could you please check the diff on PR?
https://github.com/apache/storm/pull/1939/files
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] storm pull request #1939: STORM-1363: TridentKafkaState
should handle null v...
Posted by Sachin Pasalkar <Sa...@symantec.com>.
Please review if possible. Its very small change.
On 14/02/17, 3:58 PM, "pasalkarsachin1" <gi...@git.apache.org> wrote:
>GitHub user pasalkarsachin1 opened a pull request:
>
>
>https://clicktime.symantec.com/a/1/DBEpMKi7S8ttDWQWBLAX77jjfwHUyy4Njl5cZ3H
>b8zI=?d=hjW3hU4fseqlJeekQsKUtQDmn-71nGoCmEGzTXJ7cVpl3Xhyc3GnsK37akkU6NuBPl
>pqnC9IyIIr2cmRHSEuxFgBZDez_zHyWRlstrsFpNFXaxWFVlmrYqwSUBKCDOIRL691ETp_6G_y
>axpHEDvjKXpDAKpQnQLZm6Sv6an3fYB4D61QZST5BCQBfxAlTFVaY8cliJ2lpd6g7m7-hHc9Me
>P6eGE_UjLIwX0g1QtRnEPoy7utwfYRsHl_iH5eOtjExULV8tKKFnZGnZI-q2ZQDnQcRZ-LsJfO
>t1NT_pkPjRGreRpCBQzNQ7wksgkYGpRhe_WYP3KMFdofrq19DmK0R5VsAPum4BjxhharZ2gziV
>WGO2PM_Dc63s4xX0p1eoIIlOWXI6learycQjJ1NKS0E0KDJ7PhcBAPzNjjZHGxLFuTnltKJ-bE
>7W59hXKaBHdzlHIUcuvC3uH_CMhgRyB3ohF6_ulMFFP4wngS&u=https%3A%2F%2Fgithub.co
>m%2Fapache%2Fstorm%2Fpull%2F1939
>
> STORM-1363: TridentKafkaState should handle null values from
>TridentTupleToKafkaMapper.getMessageFromTuple()
>
> In case null value comes from the mapper it will print warning
>messages.
> Added log to print the time taken to emit number of messages.
>
>You can merge this pull request into a Git repository by running:
>
> $ git pull
>https://clicktime.symantec.com/a/1/SernQA8KSlmNZQjIPv0ZkiF9eDlGpTWJdhaUYHo
>t3DY=?d=hjW3hU4fseqlJeekQsKUtQDmn-71nGoCmEGzTXJ7cVpl3Xhyc3GnsK37akkU6NuBPl
>pqnC9IyIIr2cmRHSEuxFgBZDez_zHyWRlstrsFpNFXaxWFVlmrYqwSUBKCDOIRL691ETp_6G_y
>axpHEDvjKXpDAKpQnQLZm6Sv6an3fYB4D61QZST5BCQBfxAlTFVaY8cliJ2lpd6g7m7-hHc9Me
>P6eGE_UjLIwX0g1QtRnEPoy7utwfYRsHl_iH5eOtjExULV8tKKFnZGnZI-q2ZQDnQcRZ-LsJfO
>t1NT_pkPjRGreRpCBQzNQ7wksgkYGpRhe_WYP3KMFdofrq19DmK0R5VsAPum4BjxhharZ2gziV
>WGO2PM_Dc63s4xX0p1eoIIlOWXI6learycQjJ1NKS0E0KDJ7PhcBAPzNjjZHGxLFuTnltKJ-bE
>7W59hXKaBHdzlHIUcuvC3uH_CMhgRyB3ohF6_ulMFFP4wngS&u=https%3A%2F%2Fgithub.co
>m%2Fpasalkarsachin1%2Fstorm STORM-1363
>
>Alternatively you can review and apply these changes as the patch at:
>
>
>https://clicktime.symantec.com/a/1/xOUqJRyJnT_tV6kmFKnezykmhm7Nlr271eJ8nnz
>K7Lo=?d=hjW3hU4fseqlJeekQsKUtQDmn-71nGoCmEGzTXJ7cVpl3Xhyc3GnsK37akkU6NuBPl
>pqnC9IyIIr2cmRHSEuxFgBZDez_zHyWRlstrsFpNFXaxWFVlmrYqwSUBKCDOIRL691ETp_6G_y
>axpHEDvjKXpDAKpQnQLZm6Sv6an3fYB4D61QZST5BCQBfxAlTFVaY8cliJ2lpd6g7m7-hHc9Me
>P6eGE_UjLIwX0g1QtRnEPoy7utwfYRsHl_iH5eOtjExULV8tKKFnZGnZI-q2ZQDnQcRZ-LsJfO
>t1NT_pkPjRGreRpCBQzNQ7wksgkYGpRhe_WYP3KMFdofrq19DmK0R5VsAPum4BjxhharZ2gziV
>WGO2PM_Dc63s4xX0p1eoIIlOWXI6learycQjJ1NKS0E0KDJ7PhcBAPzNjjZHGxLFuTnltKJ-bE
>7W59hXKaBHdzlHIUcuvC3uH_CMhgRyB3ohF6_ulMFFP4wngS&u=https%3A%2F%2Fgithub.co
>m%2Fapache%2Fstorm%2Fpull%2F1939.patch
>
>To close this pull request, make a commit to your master/trunk branch
>with (at least) the following in the commit message:
>
> This closes #1939
>
>----
>commit f78760c7ed94b14312d0eae1c7b5688c7eb4e96d
>Author: Sachin Pasalkar <sa...@symantec.com>
>Date: 2017-02-14T10:24:23Z
>
> STORM-1363: TridentKafkaState should handle null values from
>TridentTupleToKafkaMapper.getMessageFromTuple()
>
> Incase null value comes from the mapper it will print warning
>messages also added the time take to emit number od messages in logs
>
>----
>
>
>---
>If your project is set up for it, you can reply to this email and have
>your
>reply appear on GitHub as well. If your project does not have this feature
>enabled and wishes so, or if the feature is enabled but not working,
>please
>contact infrastructure at infrastructure@apache.org or file a JIRA ticket
>with INFRA.
>---
[GitHub] storm pull request #1939: STORM-1363: TridentKafkaState should handle null v...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1939#discussion_r101978059
--- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/trident/TridentKafkaState.java ---
@@ -98,14 +107,19 @@ public void updateState(List<TridentTuple> tuples, TridentCollector collector) {
}
if(exceptions.size() > 0){
- String errorMsg = "Could not retrieve result for messages " + tuples + " from topic = " + topic
- + " because of the following exceptions: \n";
- for (ExecutionException exception : exceptions) {
- errorMsg = errorMsg + exception.getMessage() + "\n";
- }
- LOG.error(errorMsg);
- throw new FailedException(errorMsg);
- }
+ StringBuilder errorMsg = new StringBuilder("Could not retrieve result for messages ").append(tuples)
+ .append(" from topic = ").append(topic).append(" because of the following exceptions: ").append(System.lineSeparator());
+
+ for (ExecutionException exception : exceptions) {
+ errorMsg = errorMsg.append(exception.getMessage()).append(System.lineSeparator()); ;
+ }
+ String message = errorMsg.toString();
+ LOG.error(message);
+ throw new FailedException(message);
+ }
+ long latestTime=System.currentTimeMillis();
+ LOG.info("Emitted record {} sucessfully in {} ms to topic {} ", new Object[]{emittedRecords,latestTime-currentTime,topic});
--- End diff --
Please keep log simple for INFO, since it will be called for every updates. Just leaving record count is enough.
If you really want to leave these records, this should be DEBUG.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1939: STORM-1363: TridentKafkaState should handle null v...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1939#discussion_r101973797
--- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/trident/TridentKafkaState.java ---
@@ -98,14 +107,19 @@ public void updateState(List<TridentTuple> tuples, TridentCollector collector) {
}
if(exceptions.size() > 0){
- String errorMsg = "Could not retrieve result for messages " + tuples + " from topic = " + topic
- + " because of the following exceptions: \n";
- for (ExecutionException exception : exceptions) {
- errorMsg = errorMsg + exception.getMessage() + "\n";
- }
- LOG.error(errorMsg);
- throw new FailedException(errorMsg);
- }
+ StringBuilder errorMsg = new StringBuilder("Could not retrieve result for messages ").append(tuples)
+ .append(" from topic = ").append(topic).append(" because of the following exceptions: ").append(System.lineSeparator());
+
+ for (ExecutionException exception : exceptions) {
+ errorMsg = errorMsg.append(exception.getMessage()).append(System.lineSeparator()); ;
+ }
+ String message = errorMsg.toString();
+ LOG.error(message);
+ throw new FailedException(message);
+ }
+ long latestTime=System.currentTimeMillis();
--- End diff --
Code style is not consistent even your changes. spaces before and after '='.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1939: STORM-1363: TridentKafkaState should handle null values f...
Posted by pasalkarsachin1 <gi...@git.apache.org>.
Github user pasalkarsachin1 commented on the issue:
https://github.com/apache/storm/pull/1939
Thanks for merge
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1939: STORM-1363: TridentKafkaState should handle null v...
Posted by pasalkarsachin1 <gi...@git.apache.org>.
Github user pasalkarsachin1 closed the pull request at:
https://github.com/apache/storm/pull/1939
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1939: STORM-1363: TridentKafkaState should handle null v...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1939#discussion_r101994430
--- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/trident/TridentKafkaState.java ---
@@ -98,14 +107,19 @@ public void updateState(List<TridentTuple> tuples, TridentCollector collector) {
}
if(exceptions.size() > 0){
- String errorMsg = "Could not retrieve result for messages " + tuples + " from topic = " + topic
- + " because of the following exceptions: \n";
- for (ExecutionException exception : exceptions) {
- errorMsg = errorMsg + exception.getMessage() + "\n";
- }
- LOG.error(errorMsg);
- throw new FailedException(errorMsg);
- }
+ StringBuilder errorMsg = new StringBuilder("Could not retrieve result for messages ").append(tuples)
+ .append(" from topic = ").append(topic).append(" because of the following exceptions: ").append(System.lineSeparator());
+
+ for (ExecutionException exception : exceptions) {
+ errorMsg = errorMsg.append(exception.getMessage()).append(System.lineSeparator()); ;
+ }
+ String message = errorMsg.toString();
+ LOG.error(message);
+ throw new FailedException(message);
+ }
+ long latestTime=System.currentTimeMillis();
+ LOG.info("Emitted record {} sucessfully in {} ms to topic {} ", new Object[]{emittedRecords,latestTime-currentTime,topic});
--- End diff --
My bad ignore this. I was misunderstanding it as actual payload, but was just count. It should be OK.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1939: STORM-1363: TridentKafkaState should handle null values f...
Posted by pasalkarsachin1 <gi...@git.apache.org>.
Github user pasalkarsachin1 commented on the issue:
https://github.com/apache/storm/pull/1939
@HeartSaVioR Done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1939: STORM-1363: TridentKafkaState should handle null v...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1939#discussion_r101976534
--- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/trident/TridentKafkaState.java ---
@@ -75,20 +75,29 @@ public void prepare(Properties options) {
public void updateState(List<TridentTuple> tuples, TridentCollector collector) {
String topic = null;
try {
- List<Future<RecordMetadata>> futures = new ArrayList<>(tuples.size());
+ long currentTime=System.currentTimeMillis();
+ int numberOfRecords = tuples.size();
+ List<Future<RecordMetadata>> futures = new ArrayList<>(numberOfRecords);
for (TridentTuple tuple : tuples) {
topic = topicSelector.getTopic(tuple);
-
+ Object messageFromTuple = mapper.getMessageFromTuple(tuple);
+ Object keyFromTuple = mapper.getKeyFromTuple(tuple);
+
if(topic != null) {
- Future<RecordMetadata> result = producer.send(new ProducerRecord(topic,
- mapper.getKeyFromTuple(tuple), mapper.getMessageFromTuple(tuple)));
- futures.add(result);
+ if(messageFromTuple!=null){
--- End diff --
space between ')' and '{', and left side of '!=' and right side of '!='. may be better to add space between 'f' and '('.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1939: STORM-1363: TridentKafkaState should handle null values f...
Posted by pasalkarsachin1 <gi...@git.apache.org>.
Github user pasalkarsachin1 commented on the issue:
https://github.com/apache/storm/pull/1939
I haven't changed anything it was I guess previous one had style. You can ignore white space by appending ?w=1 to url. e.g. https://github.com/apache/storm/pull/1939/files?w=1 This shows only updated code
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1939: STORM-1363: TridentKafkaState should handle null values f...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1939
+1
@pasalkarsachin1 Could you squash commits into one? Thanks in advance!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1939: STORM-1363: TridentKafkaState should handle null values f...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1939
@pasalkarsachin1
Merged into master via 313e960bedde3b2c47520a74c391425d79271727, and 1.x branch via 4fc55b33504d446ef192a25e5164f861e1495291.
Could you close this PR? Thanks in advance.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---