You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pig.apache.org by Aniket Mokashi <an...@gmail.com> on 2014/01/18 04:52:05 UTC
Review Request 17087: PIG-2207 Support custom counters for aggregating
warnings from different udfs
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17087/
-----------------------------------------------------------
Review request for pig, Cheolsoo Park, Daniel Dai, and Thejas Nair.
Bugs: PIG-2207
https://issues.apache.org/jira/browse/PIG-2207
Repository: pig-git
Description
-------
- Grouped counters for udfs
- Log at least once.
Diffs
-----
src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java 6842b10
Diff: https://reviews.apache.org/r/17087/diff/
Testing
-------
Thanks,
Aniket Mokashi
Re: Review Request 17087: PIG-2207 Support custom counters for aggregating
warnings from different udfs
Posted by Aniket Mokashi <an...@gmail.com>.
> On Jan. 22, 2014, 8:22 p.m., Daniel Dai wrote:
> > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java, line 66
> > <https://reviews.apache.org/r/17087/diff/1/?file=429825#file429825line66>
> >
> > How about holding a classname + hashcode in map instead of object? Makes me a little nervous by holding a reference of the object.
I'm using WeakHashMap (the presence of a mapping for a given key will not prevent the key from being discarded by the garbage collector), so this should be fine.
> On Jan. 22, 2014, 8:22 p.m., Daniel Dai wrote:
> > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java, line 68
> > <https://reviews.apache.org/r/17087/diff/1/?file=429825#file429825line68>
> >
> > Why not LoadFunc/StoreFunc?
LoadFunc/StoreFunc have their own ways of reporting warnings (https://github.com/apache/pig/blob/trunk/src/org/apache/pig/LoadFunc.java#L304). None of those call into PigHadoopLogger. We could have a separate jira to consolidate those.
- Aniket
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17087/#review32528
-----------------------------------------------------------
On Jan. 22, 2014, 7:24 p.m., Aniket Mokashi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17087/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2014, 7:24 p.m.)
>
>
> Review request for pig, Cheolsoo Park, Daniel Dai, and Thejas Nair.
>
>
> Bugs: PIG-2207
> https://issues.apache.org/jira/browse/PIG-2207
>
>
> Repository: pig-git
>
>
> Description
> -------
>
> - Grouped counters for udfs
> - Log at least once.
>
>
> Diffs
> -----
>
> src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java 6842b10
>
> Diff: https://reviews.apache.org/r/17087/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aniket Mokashi
>
>
Re: Review Request 17087: PIG-2207 Support custom counters for aggregating
warnings from different udfs
Posted by Daniel Dai <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17087/#review32528
-----------------------------------------------------------
src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java
<https://reviews.apache.org/r/17087/#comment61401>
How about holding a classname + hashcode in map instead of object? Makes me a little nervous by holding a reference of the object.
src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java
<https://reviews.apache.org/r/17087/#comment61400>
Why not LoadFunc/StoreFunc?
- Daniel Dai
On Jan. 22, 2014, 7:24 p.m., Aniket Mokashi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17087/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2014, 7:24 p.m.)
>
>
> Review request for pig, Cheolsoo Park, Daniel Dai, and Thejas Nair.
>
>
> Bugs: PIG-2207
> https://issues.apache.org/jira/browse/PIG-2207
>
>
> Repository: pig-git
>
>
> Description
> -------
>
> - Grouped counters for udfs
> - Log at least once.
>
>
> Diffs
> -----
>
> src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java 6842b10
>
> Diff: https://reviews.apache.org/r/17087/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aniket Mokashi
>
>
Re: Review Request 17087: PIG-2207 Support custom counters for aggregating
warnings from different udfs
Posted by Daniel Dai <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17087/#review32585
-----------------------------------------------------------
Ship it!
Ship It!
- Daniel Dai
On Jan. 22, 2014, 11:54 p.m., Aniket Mokashi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17087/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2014, 11:54 p.m.)
>
>
> Review request for pig, Cheolsoo Park, Daniel Dai, and Thejas Nair.
>
>
> Bugs: PIG-2207
> https://issues.apache.org/jira/browse/PIG-2207
>
>
> Repository: pig-git
>
>
> Description
> -------
>
> - Grouped counters for udfs
> - Log at least once.
>
>
> Diffs
> -----
>
> src/org/apache/pig/LoadFunc.java 41767fa
> src/org/apache/pig/StoreFunc.java 1fcebeb
> src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java 6842b10
>
> Diff: https://reviews.apache.org/r/17087/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aniket Mokashi
>
>
Re: Review Request 17087: PIG-2207 Support custom counters for aggregating
warnings from different udfs
Posted by Aniket Mokashi <an...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17087/
-----------------------------------------------------------
(Updated Jan. 22, 2014, 11:54 p.m.)
Review request for pig, Cheolsoo Park, Daniel Dai, and Thejas Nair.
Bugs: PIG-2207
https://issues.apache.org/jira/browse/PIG-2207
Repository: pig-git
Description
-------
- Grouped counters for udfs
- Log at least once.
Diffs (updated)
-----
src/org/apache/pig/LoadFunc.java 41767fa
src/org/apache/pig/StoreFunc.java 1fcebeb
src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java 6842b10
Diff: https://reviews.apache.org/r/17087/diff/
Testing
-------
Thanks,
Aniket Mokashi
Re: Review Request 17087: PIG-2207 Support custom counters for aggregating
warnings from different udfs
Posted by Aniket Mokashi <an...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17087/
-----------------------------------------------------------
(Updated Jan. 22, 2014, 11:53 p.m.)
Review request for pig, Cheolsoo Park, Daniel Dai, and Thejas Nair.
Changes
-------
- fixed getInstance (made thread safe).
- LoadFunc/StoreFunc warn methods work through PigHadoopLoader.
Bugs: PIG-2207
https://issues.apache.org/jira/browse/PIG-2207
Repository: pig-git
Description
-------
- Grouped counters for udfs
- Log at least once.
Diffs (updated)
-----
src/org/apache/pig/LoadFunc.java 41767fa
src/org/apache/pig/StoreFunc.java 1fcebeb
src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java 6842b10
Diff: https://reviews.apache.org/r/17087/diff/
Testing
-------
Thanks,
Aniket Mokashi
Re: Review Request 17087: PIG-2207 Support custom counters for aggregating
warnings from different udfs
Posted by Aniket Mokashi <an...@gmail.com>.
> On Jan. 22, 2014, 9:43 p.m., Daniel Dai wrote:
> > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java, line 68
> > <https://reviews.apache.org/r/17087/diff/1/?file=429825#file429825line68>
> >
> > New LoadFunc/StoreFunc might use it, right?
I mean, LoadFunc/StoreFunc does not currently use PigHadoopLogger. We can do another jira to fix that.
- Aniket
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17087/#review32558
-----------------------------------------------------------
On Jan. 22, 2014, 7:24 p.m., Aniket Mokashi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17087/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2014, 7:24 p.m.)
>
>
> Review request for pig, Cheolsoo Park, Daniel Dai, and Thejas Nair.
>
>
> Bugs: PIG-2207
> https://issues.apache.org/jira/browse/PIG-2207
>
>
> Repository: pig-git
>
>
> Description
> -------
>
> - Grouped counters for udfs
> - Log at least once.
>
>
> Diffs
> -----
>
> src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java 6842b10
>
> Diff: https://reviews.apache.org/r/17087/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aniket Mokashi
>
>
Re: Review Request 17087: PIG-2207 Support custom counters for aggregating
warnings from different udfs
Posted by Aniket Mokashi <an...@gmail.com>.
> On Jan. 22, 2014, 9:43 p.m., Daniel Dai wrote:
> > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java, line 68
> > <https://reviews.apache.org/r/17087/diff/1/?file=429825#file429825line68>
> >
> > New LoadFunc/StoreFunc might use it, right?
>
> Aniket Mokashi wrote:
> I mean, LoadFunc/StoreFunc does not currently use PigHadoopLogger. We can do another jira to fix that.
>
> Daniel Dai wrote:
> What I mean is if tomorrow a LoadFunc decide to use counter, if he choose to use PigHadoopLogger.warn, should it behave the same as EvalFunc?
So, your point is since its a singleton, its possible for LoadFunc's to PigHadoopLogger.getInstance().warn and make use of this api. I agree to your point, let me add this to the code.
By the way, LoadFunc has "public final void warn(String msg, Enum warningEnum)" that does report aggregated warnings but not through this api. I'm not sure why we did not use this api. Any thoughts?
- Aniket
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17087/#review32558
-----------------------------------------------------------
On Jan. 22, 2014, 7:24 p.m., Aniket Mokashi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17087/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2014, 7:24 p.m.)
>
>
> Review request for pig, Cheolsoo Park, Daniel Dai, and Thejas Nair.
>
>
> Bugs: PIG-2207
> https://issues.apache.org/jira/browse/PIG-2207
>
>
> Repository: pig-git
>
>
> Description
> -------
>
> - Grouped counters for udfs
> - Log at least once.
>
>
> Diffs
> -----
>
> src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java 6842b10
>
> Diff: https://reviews.apache.org/r/17087/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aniket Mokashi
>
>
Re: Review Request 17087: PIG-2207 Support custom counters for
aggregating warnings from different udfs
Posted by Aniket Mokashi <an...@gmail.com>.
What if I short-circuit them?
public final void warn(String msg, Enum warningEnum) {
PigHadoopLogger.getInstance().warn(this, msg, warningEnum);
}
On Wed, Jan 22, 2014 at 3:43 PM, Daniel Dai <da...@gmail.com> wrote:
>
>
> > On Jan. 22, 2014, 9:43 p.m., Daniel Dai wrote:
> > >
> src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java,
> line 68
> > > <
> https://reviews.apache.org/r/17087/diff/1/?file=429825#file429825line68>
> > >
> > > New LoadFunc/StoreFunc might use it, right?
> >
> > Aniket Mokashi wrote:
> > I mean, LoadFunc/StoreFunc does not currently use PigHadoopLogger.
> We can do another jira to fix that.
> >
> > Daniel Dai wrote:
> > What I mean is if tomorrow a LoadFunc decide to use counter, if he
> choose to use PigHadoopLogger.warn, should it behave the same as EvalFunc?
> >
> > Aniket Mokashi wrote:
> > So, your point is since its a singleton, its possible for LoadFunc's
> to PigHadoopLogger.getInstance().warn and make use of this api. I agree to
> your point, let me add this to the code.
> >
> > By the way, LoadFunc has "public final void warn(String msg, Enum
> warningEnum)" that does report aggregated warnings but not through this
> api. I'm not sure why we did not use this api. Any thoughts?
>
> Not sure why, but make EvalFunc standout seems not right to me in your
> method.
>
>
> - Daniel
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17087/#review32558
> -----------------------------------------------------------
>
>
> On Jan. 22, 2014, 7:24 p.m., Aniket Mokashi wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/17087/
> > -----------------------------------------------------------
> >
> > (Updated Jan. 22, 2014, 7:24 p.m.)
> >
> >
> > Review request for pig, Cheolsoo Park, Daniel Dai, and Thejas Nair.
> >
> >
> > Bugs: PIG-2207
> > https://issues.apache.org/jira/browse/PIG-2207
> >
> >
> > Repository: pig-git
> >
> >
> > Description
> > -------
> >
> > - Grouped counters for udfs
> > - Log at least once.
> >
> >
> > Diffs
> > -----
> >
> >
> src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java
> 6842b10
> >
> > Diff: https://reviews.apache.org/r/17087/diff/
> >
> >
> > Testing
> > -------
> >
> >
> > Thanks,
> >
> > Aniket Mokashi
> >
> >
>
>
--
"...:::Aniket:::... Quetzalco@tl"
Re: Review Request 17087: PIG-2207 Support custom counters for aggregating
warnings from different udfs
Posted by Daniel Dai <da...@gmail.com>.
> On Jan. 22, 2014, 9:43 p.m., Daniel Dai wrote:
> > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java, line 68
> > <https://reviews.apache.org/r/17087/diff/1/?file=429825#file429825line68>
> >
> > New LoadFunc/StoreFunc might use it, right?
>
> Aniket Mokashi wrote:
> I mean, LoadFunc/StoreFunc does not currently use PigHadoopLogger. We can do another jira to fix that.
>
> Daniel Dai wrote:
> What I mean is if tomorrow a LoadFunc decide to use counter, if he choose to use PigHadoopLogger.warn, should it behave the same as EvalFunc?
>
> Aniket Mokashi wrote:
> So, your point is since its a singleton, its possible for LoadFunc's to PigHadoopLogger.getInstance().warn and make use of this api. I agree to your point, let me add this to the code.
>
> By the way, LoadFunc has "public final void warn(String msg, Enum warningEnum)" that does report aggregated warnings but not through this api. I'm not sure why we did not use this api. Any thoughts?
Not sure why, but make EvalFunc standout seems not right to me in your method.
- Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17087/#review32558
-----------------------------------------------------------
On Jan. 22, 2014, 7:24 p.m., Aniket Mokashi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17087/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2014, 7:24 p.m.)
>
>
> Review request for pig, Cheolsoo Park, Daniel Dai, and Thejas Nair.
>
>
> Bugs: PIG-2207
> https://issues.apache.org/jira/browse/PIG-2207
>
>
> Repository: pig-git
>
>
> Description
> -------
>
> - Grouped counters for udfs
> - Log at least once.
>
>
> Diffs
> -----
>
> src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java 6842b10
>
> Diff: https://reviews.apache.org/r/17087/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aniket Mokashi
>
>
Re: Review Request 17087: PIG-2207 Support custom counters for aggregating
warnings from different udfs
Posted by Daniel Dai <da...@gmail.com>.
> On Jan. 22, 2014, 9:43 p.m., Daniel Dai wrote:
> > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java, line 68
> > <https://reviews.apache.org/r/17087/diff/1/?file=429825#file429825line68>
> >
> > New LoadFunc/StoreFunc might use it, right?
>
> Aniket Mokashi wrote:
> I mean, LoadFunc/StoreFunc does not currently use PigHadoopLogger. We can do another jira to fix that.
What I mean is if tomorrow a LoadFunc decide to use counter, if he choose to use PigHadoopLogger.warn, should it behave the same as EvalFunc?
- Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17087/#review32558
-----------------------------------------------------------
On Jan. 22, 2014, 7:24 p.m., Aniket Mokashi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17087/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2014, 7:24 p.m.)
>
>
> Review request for pig, Cheolsoo Park, Daniel Dai, and Thejas Nair.
>
>
> Bugs: PIG-2207
> https://issues.apache.org/jira/browse/PIG-2207
>
>
> Repository: pig-git
>
>
> Description
> -------
>
> - Grouped counters for udfs
> - Log at least once.
>
>
> Diffs
> -----
>
> src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java 6842b10
>
> Diff: https://reviews.apache.org/r/17087/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aniket Mokashi
>
>
Re: Review Request 17087: PIG-2207 Support custom counters for aggregating
warnings from different udfs
Posted by Daniel Dai <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17087/#review32558
-----------------------------------------------------------
src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java
<https://reviews.apache.org/r/17087/#comment61446>
Yes, you are right.
src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java
<https://reviews.apache.org/r/17087/#comment61447>
New LoadFunc/StoreFunc might use it, right?
- Daniel Dai
On Jan. 22, 2014, 7:24 p.m., Aniket Mokashi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17087/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2014, 7:24 p.m.)
>
>
> Review request for pig, Cheolsoo Park, Daniel Dai, and Thejas Nair.
>
>
> Bugs: PIG-2207
> https://issues.apache.org/jira/browse/PIG-2207
>
>
> Repository: pig-git
>
>
> Description
> -------
>
> - Grouped counters for udfs
> - Log at least once.
>
>
> Diffs
> -----
>
> src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java 6842b10
>
> Diff: https://reviews.apache.org/r/17087/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aniket Mokashi
>
>
Re: Review Request 17087: PIG-2207 Support custom counters for aggregating
warnings from different udfs
Posted by Aniket Mokashi <an...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17087/
-----------------------------------------------------------
(Updated Jan. 22, 2014, 7:24 p.m.)
Review request for pig, Cheolsoo Park, Daniel Dai, and Thejas Nair.
Bugs: PIG-2207
https://issues.apache.org/jira/browse/PIG-2207
Repository: pig-git
Description
-------
- Grouped counters for udfs
- Log at least once.
Diffs
-----
src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java 6842b10
Diff: https://reviews.apache.org/r/17087/diff/
Testing
-------
Thanks,
Aniket Mokashi
Re: Review Request 17087: PIG-2207 Support custom counters for aggregating
warnings from different udfs
Posted by Aniket Mokashi <an...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17087/
-----------------------------------------------------------
(Updated Jan. 22, 2014, 7:24 p.m.)
Review request for pig, Cheolsoo Park, Daniel Dai, and Thejas Nair.
Changes
-------
remove whitespace.
Bugs: PIG-2207
https://issues.apache.org/jira/browse/PIG-2207
Repository: pig-git
Description
-------
- Grouped counters for udfs
- Log at least once.
Diffs (updated)
-----
src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigHadoopLogger.java 6842b10
Diff: https://reviews.apache.org/r/17087/diff/
Testing
-------
Thanks,
Aniket Mokashi