Move removeRemoteSource from RemoteTaskFactory to RemoteTask #12691
Conversation
| @@ -103,9 +112,10 @@ | |||
| public final class HttpRemoteTask | |||
| implements RemoteTask | |||
| { | |||
| private static final Logger log = Logger.get(HttpRemoteTask.class); | |||
| private static final Logger LOG = Logger.get(HttpRemoteTask.class); | |||
There was a problem hiding this comment.
There are ~160 occurrences of Logger log vs ~20 occurrences of Logger LOG. I would stick with log, as upper case is rather used for constants. Logger object is not a data object, so it is hard to call it a constant.
presto-main/src/main/java/com/facebook/presto/server/remotetask/HttpRemoteTask.java
Outdated
Show resolved
Hide resolved
| Request request = prepareDelete() | ||
| .setUri(remoteSourceUri) | ||
| .build(); | ||
| RequestErrorTracker errorTracker = new RequestErrorTracker( |
There was a problem hiding this comment.
Do we need to create tracker here or maybe we should create this tracker in removeRemoteSourceHelper given the only usage is to pass this variable inside?
There was a problem hiding this comment.
@shixuan-fan : Remember removeRemoteSourceHelper recursively calls itself, so we don't want to create RequestErrorTracker inside it :)
There was a problem hiding this comment.
Cool. Didn't see that.
| return future; | ||
| } | ||
|
|
||
| private void removeRemoteSourceHelper(RequestErrorTracker errorTracker, Request request, SettableFuture<?> future) |
There was a problem hiding this comment.
This method name sounds more like it removes something called remoteSourceHelper. How about handleRemoteSourceRemoval?
There was a problem hiding this comment.
or doRemoveRemoteSource ? ;)
wenleix
force-pushed
the
refactor_remove_source
branch
3 times, most recently
from
3ca7131
to
5dad3e2
Compare
wenleix
force-pushed
the
refactor_remove_source
branch
from
5dad3e2
to
e8195d5
Compare
Loose ends as discussed in #12488 (comment) ; required by recoverable grouped execution (#12124)