[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...

2018-07-25 Thread asfgit 
Github user asfgit closed the pull request at:

https://github.com/apache/phoenix/pull/311


---

[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...

2018-07-24 Thread MrSandmanRUS 
Github user MrSandmanRUS commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/311#discussion_r204819321
  
--- Diff: bin/traceserver.py ---
@@ -116,8 +116,10 @@
 
 #" -Xdebug 
-Xrunjdwp:transport=dt_socket,address=5005,server=y,suspend=n " + \
 #" -XX:+UnlockCommercialFeatures -XX:+FlightRecorder 
-XX:FlightRecorderOptions=defaultrecording=true,dumponexit=true" + \
-java_cmd = '%(java)s $PHOENIX_OPTS ' + \
-'-cp ' + hbase_config_path + os.pathsep + 
phoenix_utils.phoenix_traceserver_jar + os.pathsep + 
phoenix_utils.phoenix_client_jar + \
+java_cmd = '%(java)s  ' + \
--- End diff --

Ok, thank you


---

[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...

2018-07-24 Thread joshelser 
Github user joshelser commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/311#discussion_r204818876
  
--- Diff: bin/traceserver.py ---
@@ -116,8 +116,10 @@
 
 #" -Xdebug 
-Xrunjdwp:transport=dt_socket,address=5005,server=y,suspend=n " + \
 #" -XX:+UnlockCommercialFeatures -XX:+FlightRecorder 
-XX:FlightRecorderOptions=defaultrecording=true,dumponexit=true" + \
-java_cmd = '%(java)s $PHOENIX_OPTS ' + \
-'-cp ' + hbase_config_path + os.pathsep + 
phoenix_utils.phoenix_traceserver_jar + os.pathsep + 
phoenix_utils.phoenix_client_jar + \
+java_cmd = '%(java)s  ' + \
--- End diff --

Actually, looks like queryserver.py doesn't include `$PHOENIX_OPTS` so 
let's just leave that to clients. `$PHOENIX_TRACESERVER_OPTS` is inlined, so 
you're good. Sorry for the confusion.


---

[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...

2018-07-24 Thread MrSandmanRUS 
Github user MrSandmanRUS commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/311#discussion_r204807188
  
--- Diff: bin/traceserver.py ---
@@ -116,8 +116,10 @@
 
 #" -Xdebug 
-Xrunjdwp:transport=dt_socket,address=5005,server=y,suspend=n " + \
 #" -XX:+UnlockCommercialFeatures -XX:+FlightRecorder 
-XX:FlightRecorderOptions=defaultrecording=true,dumponexit=true" + \
-java_cmd = '%(java)s $PHOENIX_OPTS ' + \
-'-cp ' + hbase_config_path + os.pathsep + 
phoenix_utils.phoenix_traceserver_jar + os.pathsep + 
phoenix_utils.phoenix_client_jar + \
+java_cmd = '%(java)s  ' + \
--- End diff --

Do I need to add something like this?

phoenix_opts= os.getenv('PHOENIX_OPTS', '')
..
if hbase_env.has_key('PHOENIX_OPTS'):
opts = hbase_env['PHOENIX_OPTS']
.
java_cmd = '%(java)s  ' + phoenix_opts + \



---

[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...

2018-07-24 Thread joshelser 
Github user joshelser commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/311#discussion_r204795871
  
--- Diff: bin/traceserver.py ---
@@ -116,8 +116,10 @@
 
 #" -Xdebug 
-Xrunjdwp:transport=dt_socket,address=5005,server=y,suspend=n " + \
 #" -XX:+UnlockCommercialFeatures -XX:+FlightRecorder 
-XX:FlightRecorderOptions=defaultrecording=true,dumponexit=true" + \
-java_cmd = '%(java)s $PHOENIX_OPTS ' + \
-'-cp ' + hbase_config_path + os.pathsep + 
phoenix_utils.phoenix_traceserver_jar + os.pathsep + 
phoenix_utils.phoenix_client_jar + \
+java_cmd = '%(java)s  ' + \
--- End diff --

The issue is that subprocess doesn't handle shell variable expansion "in 
the command". Subprocess has API to provide an environment to the process being 
run.

`$PHOENIX_OPTS` and `$PHOENIX_TRACESERVER_OPTS` should be placed into the 
subprocess's environment, not just dropped.


---

[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...

2018-07-18 Thread MrSandmanRUS 
Github user MrSandmanRUS commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/311#discussion_r203552899
  
--- Diff: bin/traceserver.py ---
@@ -116,8 +116,10 @@
 
 #" -Xdebug 
-Xrunjdwp:transport=dt_socket,address=5005,server=y,suspend=n " + \
 #" -XX:+UnlockCommercialFeatures -XX:+FlightRecorder 
-XX:FlightRecorderOptions=defaultrecording=true,dumponexit=true" + \
-java_cmd = '%(java)s $PHOENIX_OPTS ' + \
-'-cp ' + hbase_config_path + os.pathsep + 
phoenix_utils.phoenix_traceserver_jar + os.pathsep + 
phoenix_utils.phoenix_client_jar + \
+java_cmd = '%(java)s  ' + \
--- End diff --

All in all, the server successfully starts and works correctly.


---

[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...

2018-07-18 Thread MrSandmanRUS 
Github user MrSandmanRUS commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/311#discussion_r203552153
  
--- Diff: bin/traceserver.py ---
@@ -116,8 +116,10 @@
 
 #" -Xdebug 
-Xrunjdwp:transport=dt_socket,address=5005,server=y,suspend=n " + \
 #" -XX:+UnlockCommercialFeatures -XX:+FlightRecorder 
-XX:FlightRecorderOptions=defaultrecording=true,dumponexit=true" + \
-java_cmd = '%(java)s $PHOENIX_OPTS ' + \
-'-cp ' + hbase_config_path + os.pathsep + 
phoenix_utils.phoenix_traceserver_jar + os.pathsep + 
phoenix_utils.phoenix_client_jar + \
+java_cmd = '%(java)s  ' + \
--- End diff --

https://issues.apache.org/jira/browse/PHOENIX-2659
 I think it's your comment:
"Josh Elser added a comment - 12/Feb/16 03:23
ok, PHOENIX_OPTS isn't working because subprocess isn't popping a shell 
like sqlline does. It will automatically get added to PQS's environment 
already. We can just remove that.
Steve is spot-on for the argument parsing. It's just busted."
So, I just delete it.


---

[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...

2018-07-18 Thread MrSandmanRUS 
Github user MrSandmanRUS commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/311#discussion_r203550322
  
--- Diff: 
phoenix-tracing-webapp/src/main/java/org/apache/phoenix/tracingwebapp/http/TraceServlet.java
 ---
@@ -98,33 +108,36 @@ protected String getCount(String countby) {
 if(countby == null) {
   countby = DEFAULT_COUNTBY;
 }
-// Throws exception if the column not present in the trace table.
-MetricInfo.getColumnName(countby.toLowerCase());
 String sqlQuery = "SELECT "+countby+", COUNT(*) AS count FROM " + 
TRACING_TABLE + " GROUP BY "+countby+" HAVING COUNT(*) > 1 ";
 json = getResults(sqlQuery);
 return json;
   }
 
   //search the trace over parent id or trace id
-  protected String searchTrace(String parentId, String traceId,String 
logic) {
+  protected String searchTrace(String parentId, String traceId, String 
logic) {
+
 String json = null;
 String query = null;
 // Check the parent Id, trace id type or long or not.
 try {
+  if (parentId != null) {
 Long.parseLong(parentId);
+  }
+  if (traceId != null) {
 Long.parseLong(traceId);
+  }
 } catch (NumberFormatException e) {
-   throw new RuntimeException("The passed parentId/traceId is not 
a number.", e);
+  throw new RuntimeException("The passed parentId/traceId is not a 
number.", e);
 }
-if(!logic.equals(LOGIC_AND) || !logic.equals(LOGIC_OR)) {
-   throw new RuntimeException("Wrong logical operator passed to 
the query. Only "+ LOGIC_AND+","+LOGIC_OR+" are allowed.") ;
+if (logic != null && (!logic.equals(LOGIC_AND) && 
!logic.equals(LOGIC_OR))) {
--- End diff --

Fixed


---

[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...

2018-07-18 Thread MrSandmanRUS 
Github user MrSandmanRUS commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/311#discussion_r203550254
  
--- Diff: 
phoenix-tracing-webapp/src/main/java/org/apache/phoenix/tracingwebapp/http/TraceServlet.java
 ---
@@ -42,14 +45,21 @@
   private static Connection con;
   protected String DEFAULT_LIMIT = "25";
   protected String DEFAULT_COUNTBY = "hostname";
+  protected String DEFAULT_DESCRIPTION = "description";
   protected String LOGIC_AND = "AND";
   protected String LOGIC_OR = "OR";
   protected String TRACING_TABLE = "SYSTEM.TRACING_STATS";
 
-
+  @Override
+  public void init() {
+org.apache.hadoop.conf.Configuration conf = 
HBaseConfiguration.create();
--- End diff --

Fixed


---

[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...

2018-07-18 Thread MrSandmanRUS 
Github user MrSandmanRUS commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/311#discussion_r203550218
  
--- Diff: 
phoenix-tracing-webapp/src/main/java/org/apache/phoenix/tracingwebapp/http/Main.java
 ---
@@ -62,15 +63,18 @@ public int run(String[] arg0) throws Exception {
 final String home = getConf().get(TRACE_SERVER_HTTP_JETTY_HOME_KEY,
 DEFAULT_HTTP_HOME);
 //setting up the embedded server
-ProtectionDomain domain = Main.class.getProtectionDomain();
-URL location = domain.getCodeSource().getLocation();
-String webappDirLocation = location.toString().split("target")[0] 
+"src/main/webapp";
+String webappDirLocation = DEFAULT_WEBAPP_DIR_LOCATION;
--- End diff --

Fixed


---

[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...

2018-07-18 Thread joshelser 
Github user joshelser commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/311#discussion_r203446592
  
--- Diff: 
phoenix-tracing-webapp/src/main/java/org/apache/phoenix/tracingwebapp/http/TraceServlet.java
 ---
@@ -42,14 +45,21 @@
   private static Connection con;
   protected String DEFAULT_LIMIT = "25";
   protected String DEFAULT_COUNTBY = "hostname";
+  protected String DEFAULT_DESCRIPTION = "description";
   protected String LOGIC_AND = "AND";
   protected String LOGIC_OR = "OR";
   protected String TRACING_TABLE = "SYSTEM.TRACING_STATS";
 
-
+  @Override
+  public void init() {
+org.apache.hadoop.conf.Configuration conf = 
HBaseConfiguration.create();
--- End diff --

Import `org.apache.hadoop.conf.Configuration` please.


---

[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...

2018-07-18 Thread joshelser 
Github user joshelser commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/311#discussion_r203446437
  
--- Diff: 
phoenix-tracing-webapp/src/main/java/org/apache/phoenix/tracingwebapp/http/Main.java
 ---
@@ -62,15 +63,18 @@ public int run(String[] arg0) throws Exception {
 final String home = getConf().get(TRACE_SERVER_HTTP_JETTY_HOME_KEY,
 DEFAULT_HTTP_HOME);
 //setting up the embedded server
-ProtectionDomain domain = Main.class.getProtectionDomain();
-URL location = domain.getCodeSource().getLocation();
-String webappDirLocation = location.toString().split("target")[0] 
+"src/main/webapp";
+String webappDirLocation = DEFAULT_WEBAPP_DIR_LOCATION;
--- End diff --

Suggest dropping `webappDirLocation` and just using 
`DEFAULT_WEBAPP_DIR_LOCATION` since this is not configurable anyways.


---

[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...

2018-07-18 Thread joshelser 
Github user joshelser commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/311#discussion_r203447995
  
--- Diff: 
phoenix-tracing-webapp/src/main/java/org/apache/phoenix/tracingwebapp/http/TraceServlet.java
 ---
@@ -98,33 +108,36 @@ protected String getCount(String countby) {
 if(countby == null) {
   countby = DEFAULT_COUNTBY;
 }
-// Throws exception if the column not present in the trace table.
-MetricInfo.getColumnName(countby.toLowerCase());
 String sqlQuery = "SELECT "+countby+", COUNT(*) AS count FROM " + 
TRACING_TABLE + " GROUP BY "+countby+" HAVING COUNT(*) > 1 ";
 json = getResults(sqlQuery);
 return json;
   }
 
   //search the trace over parent id or trace id
-  protected String searchTrace(String parentId, String traceId,String 
logic) {
+  protected String searchTrace(String parentId, String traceId, String 
logic) {
+
 String json = null;
 String query = null;
 // Check the parent Id, trace id type or long or not.
 try {
+  if (parentId != null) {
 Long.parseLong(parentId);
+  }
+  if (traceId != null) {
 Long.parseLong(traceId);
+  }
 } catch (NumberFormatException e) {
-   throw new RuntimeException("The passed parentId/traceId is not 
a number.", e);
+  throw new RuntimeException("The passed parentId/traceId is not a 
number.", e);
 }
-if(!logic.equals(LOGIC_AND) || !logic.equals(LOGIC_OR)) {
-   throw new RuntimeException("Wrong logical operator passed to 
the query. Only "+ LOGIC_AND+","+LOGIC_OR+" are allowed.") ;
+if (logic != null && (!logic.equals(LOGIC_AND) && 
!logic.equals(LOGIC_OR))) {
--- End diff --

Parenthesis around `(!logic.equals(LOGIC_AND) && !logic.equals(LOGIC_OR))` 
are unnecessary, please drop them.


---

[GitHub] phoenix pull request #311: PHOENIX-4817 Fixed Phoenix Tracing Web Applicatio...

2018-07-18 Thread joshelser 
Github user joshelser commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/311#discussion_r203445596
  
--- Diff: bin/traceserver.py ---
@@ -116,8 +116,10 @@
 
 #" -Xdebug 
-Xrunjdwp:transport=dt_socket,address=5005,server=y,suspend=n " + \
 #" -XX:+UnlockCommercialFeatures -XX:+FlightRecorder 
-XX:FlightRecorderOptions=defaultrecording=true,dumponexit=true" + \
-java_cmd = '%(java)s $PHOENIX_OPTS ' + \
-'-cp ' + hbase_config_path + os.pathsep + 
phoenix_utils.phoenix_traceserver_jar + os.pathsep + 
phoenix_utils.phoenix_client_jar + \
+java_cmd = '%(java)s  ' + \
--- End diff --

Seems like `$PHOENIX_OPTS` was dropped. Was this not being interpolated 
correctly? Is this environment variable propagated into the traceserver java 
process?


---