STORM-1226: Port backtype.storm.util to java #1074
Conversation
knusbaum
changed the title
knusbaum
changed the title
knusbaum
changed the title
|
Test failures appear to be unrelated. I am +1, however I also worked on much of this. It would also be good to squash this before it is merged. |
knusbaum
force-pushed
the
util-conversion
branch
from
6e283bf to
3befae3
Compare
knusbaum
changed the title
| even though maven does not officially support ordering of the classpath.--> | ||
| <dependency> | ||
| <groupId>uk.org.lidalia</groupId> | ||
| <artifactId>sysout-over-slf4j</artifactId> |
There was a problem hiding this comment.
The new dependency seems OK, and it is unlikely that we will encounter some topology already using this, but could we please shade it anyways?
| List<String> streams = new ArrayList<>(common.get_streams().keySet()); | ||
| streamNametoId.put(name, idify(streams)); | ||
| streamIdToName.put(name, Utils.reverseMap(streamNametoId.get(name))); | ||
| //TODO: Can the call to simpleReverseMap be replaced wih Utils.reverseMap ? |
There was a problem hiding this comment.
Don't know either delete this message or replace the call.
| return Time.currentTimeSecs() - timeInSeconds; | ||
| } | ||
|
|
||
| public static long deltaMs(long timeInMilliseconds) { |
There was a problem hiding this comment.
Most other places in this file uses Millis not Ms, or milliseconds is considered the default. Could we follow and rename this deltaMillis, or rename all of the others to use Ms instead of Millis?
|
@abhishekagarwal87 @revans2 |
| (uncaughtException [this _ error] | ||
| ((:report-error <>) error) | ||
| (when (Utils/exceptionCauseIsInstanceOf ClassCastException error) | ||
| (log-message "CLASS CAST EXCEPTION WOOOOOOOOOOOOOOOO!")) |
There was a problem hiding this comment.
this is probably leftover from debugging.
| new ThriftAccessLogger().log( | ||
| String.format("Request ID: {} access from: {} principal: {} operation: {}", | ||
| requestId, remoteAddress, principal, operation)); | ||
| } |
There was a problem hiding this comment.
I thought it would be something like
LOG.info("Request ID: {} access from: {} principal: {} operation: {}",
requestId, remoteAddress, principal, operation);
and ThriftAccessLogger.log would just go away
|
One minor nit with the logging and then I am +1 |
| execCommand("kill", "-" + signum, pid); | ||
| } | ||
| } catch (ExecuteException e) { | ||
| LOG.info("Error when trying to kill " + pid + ". Process is probably already dead."); |
There was a problem hiding this comment.
Minor, but we could use {} format strings like elsewhere.
|
OK, looked again and noticed some things that might be good to address now. |
|
OK looks good I am +1 again. I would like other +1s since I contributed some of this. |
5 participants
This PR converts nearly the entirety of util.clj into Java.
What's left in util.clj will never be translated, but will disappear when the clojure code using it is rewritten in java. For now it's cleaner to leave these few functions and macros in place instead of trying to inline them everywhere.
The only operation that should need to be performed on util.clj after this PR is merged is
git rm