> On May 18, 2015, 1:38 p.m., Rajat Khandelwal wrote:
> > lens-server-api/src/main/java/org/apache/lens/server/api/common/Constant.java,
> > line 21
> > <https://reviews.apache.org/r/34210/diff/2/?file=959324#file959324line21>
> >
> > Rename this to something other than the all-generic `Constant`. It
> > doesn't yet contain every possible `Constant` to be eligible for the name
> > `Constant`.
>
> Himanshu Gahlaut wrote:
> Couldn't figure out anything common between REQUEST_ID and
> LOG_SEGREGATION_ID to assign a common name to them. Please feel free to
> suggest a name which can relate these two constants. Putting both of them in
> separate enums will be enum bloat. Hence started a generic common Constant
> enum in lens-server-api which doesn't exist right now.
so REQUEST_ID is used in only one place, it can move there.
`LOG_SEGGREGATION_ID` is only used like the following:
MDC.put(Constant.LOG_SEGREGATION_ID.getValue(), something)
or
MDC.get(Constant.LOG_SEGREGATION_ID.getValue());
These methods can be abstracted out in a new or old class(say, `LogHelper`)
which can internally store the constant.
> On May 18, 2015, 1:38 p.m., Rajat Khandelwal wrote:
> > lens-server/src/main/java/org/apache/lens/server/InitialRequestFilter.java,
> > line 41
> > <https://reviews.apache.org/r/34210/diff/2/?file=959328#file959328line41>
> >
> > It's not really a filter, going by the definition of `filter`. It's
> > merely an `interceptor`. Let's rename it to something like `request
> > enricher` or `request id appender` etc.
>
> Himanshu Gahlaut wrote:
> Jersey has a notion in which request and response can be worked upon
> using container request and response filters. The interfaces are named
> ContainerRequestFilter and ContainerResponseFilter. Hence keeping the name as
> filter to align with existing intuition. Initial keyword is used with purpose
> as this is the first filter to be called among all filters when a request is
> processed by container.
For a person trying to understand the code for the first time, the name
`Initial Request Filter` is not very descriptive. It sounds like it's filtering
an initial request. I just think that regardless of jersey naming conventions,
we should keep our code as readable as possible.
- Rajat
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34210/#review84119
-----------------------------------------------------------
On May 14, 2015, 7:36 p.m., Himanshu Gahlaut wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34210/
> -----------------------------------------------------------
>
> (Updated May 14, 2015, 7:36 p.m.)
>
>
> Review request for lens.
>
>
> Bugs: LENS-514
> https://issues.apache.org/jira/browse/LENS-514
>
>
> Repository: lens
>
>
> Description
> -------
>
> LENS-514: Adding unique identifier to log lines for log segregation
>
>
> Diffs
> -----
>
> lens-api/src/main/java/org/apache/lens/api/query/QueryHandle.java
> 7b615e6216d7a31a743416dd839a63517be88e1f
> lens-api/src/main/java/org/apache/lens/api/query/QueryPrepareHandle.java
> efa04f1c7e7fddbd9176922425c66057f4be3089
> lens-api/src/main/java/org/apache/lens/api/response/LensResponse.java
> f6c3593a131ab6779186be014d79b9f4c4cb9534
>
> lens-server-api/src/main/java/org/apache/lens/server/api/common/Constant.java
> PRE-CREATION
>
> lens-server-api/src/main/java/org/apache/lens/server/api/query/PreparedQueryContext.java
> 1ce89ac66fa817b3e47482a81b040259d2194477
>
> lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java
> 169ac8dec4b98a0cab1f652fe85afe9737f02e39
> lens-server/src/main/java/org/apache/lens/server/AuthenticationFilter.java
> b64d8223727d755aad95dee288f9561b767a5448
> lens-server/src/main/java/org/apache/lens/server/InitialRequestFilter.java
> PRE-CREATION
> lens-server/src/main/java/org/apache/lens/server/LensApplication.java
> cb452e83e6edbe4f13a3f29b4ce0cbe45effcde3
> lens-server/src/main/java/org/apache/lens/server/LensServer.java
> c6d7ea154f4c3f78eaa0675d99364a311351bf4c
> lens-server/src/main/java/org/apache/lens/server/query/QueryApp.java
> 0d23726a895a11208249b7ce458cb34e2265feeb
>
> lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
> 3bf180c9ab6f2a9fec6259848ebf65ecd147cfcf
>
> lens-server/src/main/java/org/apache/lens/server/query/QueryServiceResource.java
> 9b6d6bc223069b28367d789db847cbf05ed386c7
>
> lens-server/src/test/java/org/apache/lens/server/query/QueryAPIErrorResponseTest.java
> 5e135be0348068125b69016274e00b01140c4f21
> lens-server/src/test/resources/log4j.properties
> 65d065fa699f9797dcb13580359c4d41cfc52e9d
> tools/conf-pseudo-distr/server/log4j.properties
> afadc2f2856e7723e23265553440984bf2859869
> tools/conf/server/log4j.properties afadc2f2856e7723e23265553440984bf2859869
>
> Diff: https://reviews.apache.org/r/34210/diff/
>
>
> Testing
> -------
>
> Testing done by executing query from lens-cli and verifying lens-server logs.
>
> Running test suite. Will update test results.
>
>
> Thanks,
>
> Himanshu Gahlaut
>
>
