Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log profiling data for visualization #412

Merged
merged 99 commits into from
Mar 23, 2014
Merged

Conversation

domoritz
Copy link
Member

Tasks:

  • store all the performance log in the CSV format
  • If the query is in profiling mode, insert the performance log into master as table public#log#query-ID
  • Deprecate the python extract script, let the REST api return visualization data from master database

Unresolved issues:

  • Sqlite write lock will prevent the new logging actions.

@domoritz
Copy link
Member Author

@stechu This branch uses CSV so we should add the new log storing and copying mechanism in here.

@domoritz domoritz added the WIP label Feb 19, 2014
@stechu
Copy link
Contributor

stechu commented Feb 19, 2014

@domoritz We do not have a master database yet, which means there is no way to do the insertion on master. So I will insert the logs into worker database first.

@domoritz
Copy link
Member Author

@stechu Does that mean we need the distributed insert?

When someone needs the data, they will have to submit a query to get the logs to the master right? We could write the logs to a sqlite database on the master for now so that we don't have to pull every time.

stechu and others added 19 commits February 19, 2014 14:22
…arameters that are passed down an expression into one class.
Conflicts:
	src/edu/washington/escience/myria/expression/AndExpression.java
	src/edu/washington/escience/myria/expression/BinaryExpression.java
	src/edu/washington/escience/myria/expression/ExpressionOperator.java
	src/edu/washington/escience/myria/expression/NotExpression.java
	src/edu/washington/escience/myria/expression/OrExpression.java
	test/edu/washington/escience/myria/operator/apply/ApplyTest.java
@domoritz
Copy link
Member Author

Thanks. Works like a charm. Removed WIP.

@domoritz domoritz changed the title [WIP] Log profiling data for visualization Log profiling data for visualization Mar 20, 2014
try {
accessMethod.createIndexes(MyriaConstants.PROFILING_RELATION, MyriaConstants.PROFILING_SCHEMA, index.build());
} catch (DbException e) {
LOGGER.info(String.format("Tried to create index for profiling logs: %s", e.getMessage()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would have expected something like

LOGGER.error("Couldn't create index for profiling logs:", e);
  1. Seems worth higher priority than info
  2. Uses [the below][1] to automatically format the error

[1] : http://www.slf4j.org/api/org/slf4j/Logger.html#error(java.lang.String, java.lang.Throwable)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm totally not happy with this anyway. I would like to be able to use CREATE INDEX IF NOT EXISTS but there is no such command in Postgres and MySQL.

I changed the code to log an error instead of just info.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. And we don't want to create the table when the worker is run the first time, probably because we don't have any mechanism for that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could see whether the table exists and create only create the table + index in this case. In any case, I am not happy with this but I don't think it's worth trying to make this perfect.

* @param statement the JDBC statement
* @throws SQLException when setting values in the statement fails
*/
public void getIntoJdbc(final PreparedStatement statement) throws SQLException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this function used? I noticed it doesn't @Override anything but also getIntoJdbc does not appear elsewhere in the diff.

@@ -26,11 +26,13 @@ public QueryStatusEncoding(final long queryId) {
* @return a QueryStatusEncoding object containing the submitted query, with the submit time set to
* DateTimeUtils.nowInISO8601().
*/
public static QueryStatusEncoding submitted(final String rawQuery, final String logicalRa, final Object physicalPlan) {
public static QueryStatusEncoding submitted(final String rawQuery, final String logicalRa, final Object physicalPlan,
final Boolean profilingMode) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing @param?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where? Are you looking at a commit or the whole patch?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put it in yesterday

4304c9e

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dhalperi
Copy link
Member

I merged master because that one stupid test keeps failing, so I merged in the test-fix. :)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.21%) when pulling 37844bc on domoritz-visualization into 38670e9 on master.

dhalperi and others added 9 commits March 20, 2014 17:43
Use Preconditions.checkArgument and disambiguate error messages.
This reverts commit b9dc910.

The logic was wrong -- the 0 is misinterpreted. Instead, introduce the
static int[][]{{0}} that is the collector partition->channel mapping.
They don't pass reliably enough, so I'm disabling them until we have
someone who's interested in fixing them.
@dhalperi
Copy link
Member

Looks like there must still be some race conditions or other timeout issues in the server, because the json submit test fails but now with timeout exceptions. Hmm

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.23%) when pulling f50f305 on domoritz-visualization into 38670e9 on master.

dhalperi added a commit that referenced this pull request Mar 23, 2014
@dhalperi dhalperi merged commit 8064e2e into master Mar 23, 2014
@dhalperi dhalperi deleted the domoritz-visualization branch March 23, 2014 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants