Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Roger Hoover]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/
---

(Updated July 22, 2015, 4:07 a.m.)


Review request for samza.


Repository: samza


Description
---

SAMZA-733 Add metrics to Elasticsearch System Producer


Diffs (updated)
-

  samza-core/src/main/java/org/apache/samza/metrics/MetricGroup.java 
PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java 
PRE-CREATION 
  samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala 
8eac8ef 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
 a277b69 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
 7eb14a2 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
 PRE-CREATION 
  
samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetricsTest.java
 PRE-CREATION 
  
samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerTest.java
 e63d62c 

Diff: https://reviews.apache.org/r/36473/diff/


Testing
---

Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
stream and that the metrics correctly count how many Elasticsearch documents 
were created and indexed.


Thanks,

Roger Hoover

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Roger Hoover]

> On July 21, 2015, 5:42 p.m., Yan Fang wrote:
> > samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala, 
> > lines 36-37
> > 
> >
> > though it works, prefer to use the "def" here, not only because it has 
> > leff overhead, but also keep all the methods consistent for better 
> > readability. What do you think?
> 
> Roger Hoover wrote:
> Sounds good.  I only baulked on it the first time because I'm not that 
> skilled with Scala type decarations yet. :)  I can make this work

I take it back.  It seems it [can't be 
done](http://www.scala-lang.org/old/node/5082)


- Roger


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/#review92429
---


On July 21, 2015, 5:41 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36473/
> ---
> 
> (Updated July 21, 2015, 5:41 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-733 Add metrics to Elasticsearch System Producer
> 
> 
> Diffs
> -
> 
>   samza-core/src/main/java/org/apache/samza/metrics/MetricGroup.java 
> PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala 
> 8eac8ef 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
>  a277b69 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
>  7eb14a2 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
>  PRE-CREATION 
>   
> samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetricsTest.java
>  PRE-CREATION 
>   
> samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerTest.java
>  e63d62c 
> 
> Diff: https://reviews.apache.org/r/36473/diff/
> 
> 
> Testing
> ---
> 
> Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
> stream and that the metrics correctly count how many Elasticsearch documents 
> were created and indexed.
> 
> 
> Thanks,
> 
> Roger Hoover
> 
>

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Roger Hoover]

> On July 21, 2015, 5:42 p.m., Yan Fang wrote:
> > samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java, line 23
> > 
> >
> > of the class "the" extends -> of the class "that" extends

Thanks


> On July 21, 2015, 5:42 p.m., Yan Fang wrote:
> > samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java, line 29
> > 
> >
> > can we also have a constructor with the default prefix ""?

Good idea


> On July 21, 2015, 5:42 p.m., Yan Fang wrote:
> > samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala, 
> > lines 36-37
> > 
> >
> > though it works, prefer to use the "def" here, not only because it has 
> > leff overhead, but also keep all the methods consistent for better 
> > readability. What do you think?

Sounds good.  I only baulked on it the first time because I'm not that skilled 
with Scala type decarations yet. :)  I can make this work


- Roger


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/#review92429
---


On July 21, 2015, 5:41 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36473/
> ---
> 
> (Updated July 21, 2015, 5:41 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-733 Add metrics to Elasticsearch System Producer
> 
> 
> Diffs
> -
> 
>   samza-core/src/main/java/org/apache/samza/metrics/MetricGroup.java 
> PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala 
> 8eac8ef 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
>  a277b69 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
>  7eb14a2 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
>  PRE-CREATION 
>   
> samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetricsTest.java
>  PRE-CREATION 
>   
> samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerTest.java
>  e63d62c 
> 
> Diff: https://reviews.apache.org/r/36473/diff/
> 
> 
> Testing
> ---
> 
> Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
> stream and that the metrics correctly count how many Elasticsearch documents 
> were created and indexed.
> 
> 
> Thanks,
> 
> Roger Hoover
> 
>

Re: Samza and sliding window

[Yi Pan]

Hi, Shekar,

I have strip down your use case just to the KV-store operation and have
verified that it works fine. Please see the attached diff file.

If you have any further questions, please let me know.

-Yi

On Mon, Jul 20, 2015 at 12:35 PM, Shekar Tippur  wrote:

> Yi,
>
> Here is the config:
> http://pastebin.com/mCALEACs
>
> - Shekar
>
> On Mon, Jul 20, 2015 at 12:27 PM, Yi Pan  wrote:
>
> > Hi, Shekar,
> >
> > It would also be helpful if you can post your job configuration on the
> > pastebin s.t. I can test the same config.
> >
> > Thanks!
> >
> > -Yi
> >
> > On Mon, Jul 20, 2015 at 11:11 AM, Shekar Tippur 
> wrote:
> >
> > > Yi,
> > >
> > > Thanks a lot.
> > >
> > > - Shekar
> > >
> >
>

Re: Review Request 36006: Writing a tool to read from the coordinator stream and react to config changes accordingly.

[Navina Ramesh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36006/#review92227
---


Ideally, the job coordinator should act as the config manager. Since the chain 
of control is still from AppMaster to JobCoordinator (instead of the other way 
around), the only reasonable way to get this working is to have it as a 
separate script (like run-config-manager.sh). Having said this, I wonder if it 
is necessary for the ConfigManger to remain in samza-core. @Shadi: Were there 
any issues in keeping this code within the auto-scaling module?


samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java
 (line 47)


Remove commented lines



samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java
 (line 90)


RM_ADDRESS_OPT, RM_PORT_OPT, POLLING_INTERVAL_OPT, SERVER_URL_OPT can be 
made private



samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java
 (line 151)


Use logger instance



samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java
 (line 215)


nit: typo in "server ural"



samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java
 (line 236)


We should, in the future, change the handlers to implement a common 
interface. This way, users can register custom handlers and also, perhaps 
precondition checks. Whether that will open up to mishandling of config changes 
is still up to debate. Just wanted to comment this here.



samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java
 (line 310)


Can you add a check here to verify that the job ha actually been killed? 
You can use the rest api to verify that the status of the job has been changed 
to "KILLED".



samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java
 (line 317)


I think it is fine to skip messages written to the stream after you have 
killed a job's application attempt and before the next attempt starts up. 
However you can't avoid the bootstrap code from reading these messages. I don't 
think there is clean solution for this here.



samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java
 (line 330)


Can you move the yarn rest api related code to a separate YarnUtils class? 
Makes the code more cleaner and it can be re-used in other places in the future.



samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java
 (line 392)


Why do you need this class here?



samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala (line 52)


Can you please add a comment here for what "isFirstTime" is used for?



samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMaster.scala (line 
123)


Code here looks kind of arbitary here. Looks like you want to write the 
jobcoordinator url to the coordinator stream. You should perhaps move this to 
the init method in SamzaAppMasterService??
This way you don't have to change the interface of the run method.


- Navina Ramesh


On July 18, 2015, 2:52 a.m., Shadi A. Noghabi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36006/
> ---
> 
> (Updated July 18, 2015, 2:52 a.m.)
> 
> 
> Review request for samza, Yi Pan (Data Infrastructure), Navina Ramesh, and 
> Naveen Somasundaram.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> After a job is submitted, it might need some configuration change, 
> specifically it might need more containers. In SAMZA-704 a tool is being 
> added to write to the coordinator stream (CoordinatorStreamWriter).  This 
> tool can be used to write new configurations to the coordinator stream. 
> However, another tool (ConfigManager) is needed to read the config changes 
> and react to them, which is the goal of this task. This tool should be 
> brought up after the job is submitted and read any config changes added to 
> the coordinator stream, and react to each accordingly. 
> 
> This tool, called the Config Manager, is focusing on handling container 
> changs by reacting to set-config massages with key "yarn.container.count

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Yan Fang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/#review92429
---



samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java (line 23)


of the class "the" extends -> of the class "that" extends



samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java (line 29)


can we also have a constructor with the default prefix ""?



samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala (line 29)


the extends -> that extends



samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala (lines 
36 - 37)


though it works, prefer to use the "def" here, not only because it has leff 
overhead, but also keep all the methods consistent for better readability. What 
do you think?


- Yan Fang


On July 21, 2015, 5:41 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36473/
> ---
> 
> (Updated July 21, 2015, 5:41 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-733 Add metrics to Elasticsearch System Producer
> 
> 
> Diffs
> -
> 
>   samza-core/src/main/java/org/apache/samza/metrics/MetricGroup.java 
> PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala 
> 8eac8ef 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
>  a277b69 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
>  7eb14a2 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
>  PRE-CREATION 
>   
> samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetricsTest.java
>  PRE-CREATION 
>   
> samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerTest.java
>  e63d62c 
> 
> Diff: https://reviews.apache.org/r/36473/diff/
> 
> 
> Testing
> ---
> 
> Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
> stream and that the metrics correctly count how many Elasticsearch documents 
> were created and indexed.
> 
> 
> Thanks,
> 
> Roger Hoover
> 
>

Review Request 36641: SAMZA-739 Change version of hello-samza in SAMZA tutorials/latest

[Aleksandar Pejakovic]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36641/
---

Review request for samza.


Repository: samza


Description
---

Changed version of hello-samza in SAMZA/tutorials from 0.8.0 to 0.9.1.


Diffs
-

  docs/learn/tutorials/versioned/deploy-samza-job-from-hdfs.md 77a1e21 
  docs/learn/tutorials/versioned/deploy-samza-to-CDH.md 8c19150 
  docs/learn/tutorials/versioned/remote-debugging-samza.md 5f4f993 
  docs/learn/tutorials/versioned/run-in-multi-node-yarn.md 312efaf 

Diff: https://reviews.apache.org/r/36641/diff/


Testing
---


Thanks,

Aleksandar Pejakovic