[LENS-1545]: Fixed a Bug and made changes for PreparedQuery. #34

RajashekharChoukimath · 2019-07-03T13:18:42Z

Fixed following Issues in Lens for Azure Migrations

Bug in Lens-Metastore for single fact granularity -- https://issues.apache.org/jira/browse/LENS-1544. Explaination: This had a special case for handling facts with one-update-period and multiple-update-periods. The case which handled one-update-period had a bug while generating the name of the hive table whole format is update_period-fact_name, the update_period was coming as null.
JDBCDriver.java was invoking JDBC Driver while validating the prepared query, I don't need this functionality so blocking this via already existing config parameter JDBC_VALIDATE_THROUGH_PREPARE.
2a) Old Flow:
esitmate()
--- | --> validate
-----| --> JDBC_VALIDATE_THROUGH_PREPARE condition to prepare through if condition
-----|--> prepareInternal()
----------------|--> rewrite -- Rewritting take place
2b) New Flow:
esitmate()
---| --> rewrite -- Rewritting take place
---| --> validate
-----| --> JDBC_VALIDATE_THROUGH_PREPARE condition to prepare through if condition
-----|--> prepareInternal()
2c) JDBC_VALIDATE_THROUGH_PREPARE has been renamed as JDBC_VALIDATE_THROUGH_PREPARE_OR_EXPLAIN, because even explain makes JDBC call to the underlying JDBCDriver, so used this variable to stop it from calling underlying JDBCDriver.

File level explainations:

CubeMetastoreClient.java -- This had a special case for handling facts with one-update-period and multiple-update-periods. The case which handled one-update-period had a bug while generating the name of the hive table whole format is update_period-fact_name, the update_period was coming as null.
JDBCDriver.java -- This was trying to invoke the underlying JDBC Driver while validating the prepared query, I have moved it up into the estimate() method
TestJDBCDriver.java -- related to item 2.
PreparedQueryContext.java -- We want to store the prepared query into the DB similar to finished_queries, so added start and end times. Also used these fields to measure the time lens takes to prepare the query. Related to item 5
LensServerDAO.java -- We want to store the prepared query into the DB similar to finished_queries, so we created a new table called prepared_queries and added the cube and driver query into it.
QueryExecutionServiceImpl.java --
6a) Related to item 5. -- Inserting the prepared Query and time difference.
6b) also removed the line prepared.getSelectedDriver().prepare(prepared); because this was already taken care in prepareQuery(sessionHandle, query, lensConf, SubmitOp.PREPARE);

…onal xml tag valid.columns

…querying

…underlying JDBC Driver

Amareshwari · 2019-07-05T07:58:17Z

@RajashekharChoukimath Can you create corresponding jira and update pull request summary with jira id?

guptapuneet · 2019-07-08T03:23:40Z

@RajashekharChoukimath can you please remove the internal JIRA details from description.

RajashekharChoukimath · 2019-07-08T05:50:20Z

I will do it today. Thanks Amareshwari, Puneet.

…

On Mon, Jul 8, 2019 at 8:53 AM guptapuneet ***@***.***> wrote: @RajashekharChoukimath <https://github.com/RajashekharChoukimath> can you please remove the internal JIRA details from description. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#34?email_source=notifications&email_token=AJUFZ3NYN2UPF2ZCCRS5FOTP6KXL5A5CNFSM4H5FXXC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZL3RFQ#issuecomment-509065366>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJUFZ3PFXMIIE5GXJMVVUTDP6KXL5ANCNFSM4H5FXXCQ> .

-- with regards Rajashekhar

ankitkailaswar · 2019-07-09T10:41:36Z

lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java


    return result;
  }
+


add update prepare query also

We don't update the prepared query, we insert only prepared query, so it is not needed.

ankitkailaswar · 2019-07-09T10:41:53Z

lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java

+   * @param preparedQueryContext to be inserted
+   * @throws SQLException the exception
+   */
+  public void insertPreparedQuery(PreparedQueryContext preparedQueryContext) throws SQLException {


add unit tests

This is a void method, and anyways it calls external system to insert data, I don't think we should be unit-testing here.

I am asking or unit test around new snippet of code written here and unit test is not only around return value.
Consider a scenario where in future any user wants to use this function and end up calling "insert prepare" twice from his code then this code is expected throw exception in the second call. A developer will not come to know this at build time unless unit tests are added. There can be many such scenarios unknown now.
Also please add checks and unit tests for invalid values of preparedQueryContext. It can cause runtime exception. rethrow LensException instead.

Some one calling once/twice/zero times is not part of unit testing. It is a overkill to unnecessarily mock and unit test.
In this code snippet, it is making a call to a external system and it returns void.

anyways I have added unit test case.

ankitkailaswar · 2019-07-09T10:51:59Z

lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java

      prepared = prepareQuery(sessionHandle, query, lensConf, SubmitOp.PREPARE);
      prepared.setQueryName(queryName);
-      prepared.getSelectedDriver().prepare(prepared);
+      try {


push start time and end time for prepared query in db and emit metric for query prepare time.

For the prepared query, I am inserting start_time and timeTaken(this can deduce the end_time if needed).
Regarding the NumberOfPrepared queries, it is already being emitted PREPARED_QUERIES_COUNTER. Regarding the query prepare time is already present in database.

I assume we are capturing query prepare time for lens query to monitor its performance. If that's the case its always better to emit metric that indicates average prepare query time over last 5 mins, 60 min, etc. PREPARED_QUERIES_COUNTER is different counter I am asking you to emit one for prepare time with aggregation that can be used to raise an alert. To monitor performance by looking into db is not good practice. It is being used mostly for bookkeeping.

This is already happening. All the public methods of QueryServiceResource histograms are already pushed by default.

ankitkailaswar · 2019-07-09T11:31:06Z

lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java

    // Only create a prepared statement and then close it
    MethodMetricsContext sqlRewriteGauge = MethodMetricsFactory.createMethodGauge(pContext.getDriverConf(this), true,
      metricCallStack + COLUMNAR_SQL_REWRITE_GAUGE);
-    String rewrittenQuery = rewriteQuery(pContext);


rewrite query is called for estimate, prepare, explain etc. In this case estimate will work fine. explain will fail to initialize "String rewrittenQuery" since rewriteQuery was never called and pContext.getSelectedDriverQuery(); will be null.
Exsisting unit test must have failed for this scenario. Please run unit tests.
Also I wouldnt recoomend this approach since we have to call rewrite for estimate/prepare/explaing etc.
Please make it config driven in estimate call.

Explain also works fine because of the below stack trace:
JDBCDriver.rewriteQuery(AbstractQueryContext) line: 529
JDBCDriver.explain(AbstractQueryContext) line: 582
QueryExecutionServiceImpl.explain(String, LensSessionHandle, String, LensConf) line: 3141

Explain was also try to make JDBC connection, I have stopped this using JDBC_VALIDATE_THROUGH_PREPARE(I have renamed this variable to JDBC_VALIDATE_THROUGH_PREPARE_OR_EXPLAIN).

ankitkailaswar · 2019-07-15T06:59:47Z

lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java

      prepared = prepareQuery(sessionHandle, query, lensConf, SubmitOp.PREPARE);
      prepared.setQueryName(queryName);
-      prepared.getSelectedDriver().prepare(prepared);
+      try {


I assume we are capturing query prepare time for lens query to monitor its performance. If that's the case its always better to emit metric that indicates average prepare query time over last 5 mins, 60 min, etc. PREPARED_QUERIES_COUNTER is different counter I am asking you to emit one for prepare time with aggregation that can be used to raise an alert. To monitor performance by looking into db is not good practice. It is being used mostly for bookkeeping.

ankitkailaswar · 2019-07-15T07:32:48Z

lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java

+   * @param preparedQueryContext to be inserted
+   * @throws SQLException the exception
+   */
+  public void insertPreparedQuery(PreparedQueryContext preparedQueryContext) throws SQLException {


I am asking or unit test around new snippet of code written here and unit test is not only around return value.
Consider a scenario where in future any user wants to use this function and end up calling "insert prepare" twice from his code then this code is expected throw exception in the second call. A developer will not come to know this at build time unless unit tests are added. There can be many such scenarios unknown now.
Also please add checks and unit tests for invalid values of preparedQueryContext. It can cause runtime exception. rethrow LensException instead.

Amareshwari · 2019-07-19T06:58:39Z

lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java

+      try {
+        lensServerDao.insertPreparedQuery(prepared);
+      } catch (Exception e) {
+        incrCounter(PREPARED_QUERY_INSERT_COUNTER);


This should be a failed counter? Also why is exception eaten up?

the counter name was wrong, i have renamed it to PREPARED_QUERY_INSERT_ERROR_COUNTER and also throwed the exception.

Amareshwari · 2019-07-19T07:01:30Z

lens-cube/src/main/java/org/apache/lens/cube/metadata/CubeMetastoreClient.java

-          for (Map.Entry entry : updatePeriodToTableMap.entrySet()) {
-            XUpdatePeriodTableDescriptor updatePeriodTableDescriptor = new XUpdatePeriodTableDescriptor();
-            updatePeriodTableDescriptor.setTableDesc(getStorageTableDescFromHiveTable(
+        XStorageTableElement tblElement = new XStorageTableElement();


Can a unit test be added for the bug fixed here?

RajashekharInmobi and others added 16 commits July 18, 2018 16:04

exposing jmx port for monitoring the threads, cpu and memory

98ab7a5

Fix for RNA-10440: Lens driver not picking hadoop conf

d3b41b3

added the files

69e9e64

Merge remote-tracking branch 'upstream/master'

616aeec

Adding support for Partition Columns to queried without giving additi…

3dffffb

…onal xml tag valid.columns

reverting old change

7783450

resolved imports

149a192

added Example for supporting the non-timed partition as filter while …

c7aa785

…querying

Merge remote-tracking branch 'upstream/master'

787c7e2

Merge remote-tracking branch 'upstream/master'

1dc444c

fixed the yaml

c0cb51a

licence

65174d2

Merge remote-tracking branch 'upstream/master'

dbbf4f5

Merge remote-tracking branch 'upstream/master'

8e921d5

made few changes to the code for azure deployment

0519d9d

Fixed the test case, removed instances where Prepare was hitting the …

bc6f628

…underlying JDBC Driver

RajashekharInmobi added 4 commits July 8, 2019 16:55

reversed few changes

2d6fb3d

removing not needed commits

0caecd7

undid the unit test cases

2ce78dc

made one more change

2400d8e

RajashekharChoukimath changed the title ~~Fixed Issues in Lens for Azure Migrations~~ Fixed a Bug and made changes for PreparedQuery. Jul 9, 2019

RajashekharChoukimath changed the title ~~Fixed a Bug and made changes for PreparedQuery.~~ [LENS-1545]: Fixed a Bug and made changes for PreparedQuery. Jul 9, 2019

ankitkailaswar reviewed Jul 10, 2019

View reviewed changes

ankitkailaswar suggested changes Jul 15, 2019

View reviewed changes

RajashekharInmobi added 3 commits July 15, 2019 13:32

fixed code review comments

5fa2800

fixed code review comments

926b604

fixed code review comments

353dd56

RajashekharInmobi added 4 commits July 16, 2019 11:36

Merge branch 'master' of https://github.com/RajashekharInmobi/lens

b183d21

made some changes

fc4358e

Merge branch 'master' of https://github.com/RajashekharInmobi/lens

75c440d

added the files

fe57a2e

ankitkailaswar approved these changes Jul 16, 2019

View reviewed changes

Amareshwari reviewed Jul 19, 2019

View reviewed changes

RajashekharInmobi added 5 commits July 19, 2019 15:15

added the code review fix

0cdd720

added a unit test case

dc67c27

added the test case

8d3e015

fixed unit test case

af1f5d4

fixed unit test case

24783d6

Amareshwari approved these changes Jul 24, 2019

View reviewed changes

Amareshwari merged commit 4907964 into apache:master Jul 24, 2019

RajashekharChoukimath mentioned this pull request Aug 6, 2019

[LENS-1545]: Fixed Bug in Prepared Query. #35

Open

RajashekharInmobi added a commit to RajashekharInmobi/lens that referenced this pull request Aug 13, 2019

Changes in JDBCDriver.java was supposed to happen in apache#34,

37fdb92

[LENS-1545]: Fixed a Bug and made changes for PreparedQuery. #34

[LENS-1545]: Fixed a Bug and made changes for PreparedQuery. #34

Uh oh!

Conversation

RajashekharChoukimath commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amareshwari commented Jul 5, 2019

Uh oh!

guptapuneet commented Jul 8, 2019

Uh oh!

RajashekharChoukimath commented Jul 8, 2019 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RajashekharChoukimath commented Jul 3, 2019 •

edited

Loading