Skip to content

[WIP][TEZ-3904] An API to update tokens for Tez AM and the DAG#22

Open
beltran wants to merge 1 commit intoapache:masterfrom
beltran:TEZ-3904
Open

[WIP][TEZ-3904] An API to update tokens for Tez AM and the DAG#22
beltran wants to merge 1 commit intoapache:masterfrom
beltran:TEZ-3904

Conversation

@beltran
Copy link
Copy Markdown
Contributor

@beltran beltran commented Jun 13, 2018

This PR has ended up bigger than what I expected so I wanted to ask for some feedback before going forward. This is what is does mainly:

  • Adds updateAMCredentials and updateDAGCredentials to TezClient. This is not the final API but this functions will be useful anyway to send the credentials
  • Add two corresponding functions to the DAGClientAMProtocol
  • Add a SystemEventHandler in the runtime internals to process the UpdateCredentialsEvents
  • Add the UpdateCredentialsEvent event and some transitions to the TaskImpl's state machine that represent the credentials being updated.

TODO:

  • Add more tests. Will do after the feedback
  • Log to history the credentials change?
  • I've added some logic to renew the session credentials, these credentials should be updated as well?

String pid, ExecutionContext ExecutionContext, long memAvailable,
boolean updateSysCounters, HadoopShim hadoopShim,
TezExecutors sharedExecutor) throws IOException {
TezExecutors sharedExecutor, SystemEventHandler systemEventHandler) throws IOException {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This API changes, this is bad?

ExecutionContext executionContext, long memAvailable,
boolean updateSysCounters, HadoopShim hadoopShim,
TezExecutors sharedExecutor) throws
TezExecutors sharedExecutor, SystemEventHandler systemEventHandler) throws
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same with this API


@Private
@VisibleForTesting
public static Credentials createAMCredentials(ApplicationId appId,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I pulled this out because I was going to reuse but will leave it back in if I don't

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.

1 participant