test commit #34148
test commit #34148
Conversation
| @@ -17,152 +17,18 @@ | |||
| */ | |||
There was a problem hiding this comment.
i think this needs a rebase since theres merge conflicts now
There was a problem hiding this comment.
would recommend
- creating a dev branch
- moving these changes into it
- keeping
parveensania/masterclean and up to date w/apache/masterso that you can rebase dev branch onto master changes
| @@ -46,6 +48,7 @@ public abstract class WindmillEndpoints { | |||
| .setVersion(Long.MAX_VALUE) | |||
| .setWindmillEndpoints(ImmutableSet.of()) | |||
| .setGlobalDataEndpoints(ImmutableMap.of()) | |||
| .setEndpointType(EndpointType.UNKNOWN) | |||
There was a problem hiding this comment.
should we default to cloud path? unless the code knows how to deal with UNKNOWN?
| @@ -155,6 +163,15 @@ private static Optional<HostAndPort> tryParseDirectEndpointIntoIpV6Address( | |||
| */ | |||
| public abstract ImmutableSet<Endpoint> windmillEndpoints(); | |||
|
|
|||
| public abstract EndpointType endpointType(); | |||
|
|
|||
| public enum EndpointType { | |||
There was a problem hiding this comment.
Can we just use the EndpointType enum in the proto?
| this.heartbeatSender), | ||
| getWorkStreamLatencies); | ||
| }); | ||
| this.getWorkStream = |
There was a problem hiding this comment.
we need something like
that manages the GetWorkStream so we get clean stream closure from the client.|
|
||
| @Override | ||
| public synchronized void close() { | ||
| if (started) { |
There was a problem hiding this comment.
StreamPool will need to be modified to clean up any dangling streams.
right now we only clean up the stream on getStream()/releaseStream().
| @Internal | ||
| @ThreadSafe | ||
| final class WindmillStreamPoolSender implements WindmillStreamSender, StreamSender { | ||
| // private static final java.util.logging.Logger LOG = |
There was a problem hiding this comment.
remove the commented out LOG
| this.getWorkBudget = getWorkBudget; | ||
| this.streamingEngineThrottleTimers = StreamingEngineThrottleTimers.create(); | ||
| /** Superclass for stream senders used to communicate with Windmill */ | ||
| public interface WindmillStreamSender extends GetWorkBudgetSpender { |
There was a problem hiding this comment.
I think we can move all of this to StreamSender or just have WindmillStreamSender have the 1 method in the StreamSender interface. don't think we need both interfaces.
| @@ -127,7 +141,13 @@ private FanOutStreamingEngineWorkerHarness( | |||
| GrpcDispatcherClient dispatcherClient, | |||
| Function<WindmillStream.CommitWorkStream, WorkCommitter> workCommitterFactory, | |||
| ThrottlingGetDataMetricTracker getDataMetricTracker, | |||
| ExecutorService workerMetadataConsumer) { | |||
| ExecutorService workerMetadataConsumer, | |||
There was a problem hiding this comment.
instead of passing all of these in can the constructor take in a
Function<WindmillConnection, WindmillStreamSender> windmillStreamPoolSenderFactory
or create a WindmillStreamPoolSenderFactory.java interface
@FunctionalInterface
interface WindmillStreamPoolSenderFactory {
WindmillStreamSender create(WindmillConnection connection);
}
then in StreamingDataflowWorker.java you can pass in
connection -> WindmillStreamPoolSender.create(
connection,
GetWorkRequest.newBuilder()
.setClientId(jobHeader.getClientId())
.setJobId(jobHeader.getJobId())
.setProjectId(jobHeader.getProjectId())
.setWorkerId(jobHeader.getWorkerId())
.setMaxItems(totalGetWorkBudget.items())
.setMaxBytes(totalGetWorkBudget.bytes())
.build(),
GetWorkBudget.noBudget(),
streamFactory,
streamPoolWorkCommitter,
streamPoolGetDataClient,
streamPoolHeartbeatSender,
streamingWorkScheduler,
waitForResources,
computationStateFetcher)
and in createAndStartWindmillStreamSender
you can call windmillStreamPoolSenderFactory.create() / windmillStreamPoolSenderFactory.apply(connection)
since it seems like all of those params are just added to create the WindmillStreamPoolSender
Please add a meaningful description for your change here
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.