Skip to content

Conversation

@jose-torres
Copy link
Contributor

What changes were proposed in this pull request?

Refactor continuous writing to its own class.

See WIP jose-torres#13 for the overall direction this is going, but I think this PR is very isolated and necessary anyway.

How was this patch tested?

existing unit tests - refactoring only

@jose-torres
Copy link
Contributor Author

@tdas

@SparkQA
Copy link

SparkQA commented Apr 20, 2018

Test build #89654 has finished for PR 21116 at commit 3d8dfa4.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

@xuanyuanking xuanyuanking left a comment

Choose a reason for hiding this comment

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

Cool refactor, more clear logic of CP plans, just a nit.

val epochCoordinator = EpochCoordinatorRef.get(
context.getLocalProperty(ContinuousExecution.EPOCH_COORDINATOR_ID_KEY),
SparkEnv.get)
val currentMsg: WriterCommitMessage = null
Copy link
Member

Choose a reason for hiding this comment

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

currentMsg is no longer needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. I see its no long used anywhere. That raises two questions.

  • This should be removed.
  • Does this continuous code path not have to send back a WriterMessage? How is that working?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm. I see the msg being sent back using the epochCoordinator. Then lets just remove the currentMsg

@tdas
Copy link
Contributor

tdas commented Apr 23, 2018

LGTM! Correction found a few nits.

}

val rdd = query.execute()
val messages = new Array[WriterCommitMessage](rdd.partitions.length)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really needed. The only use of it is in the logInfo before, that too, only in the length, which is effectively rdd.partitions.length.

@jose-torres
Copy link
Contributor Author

addressed comments

@SparkQA
Copy link

SparkQA commented Apr 24, 2018

Test build #89747 has finished for PR 21116 at commit b676dc8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@asfgit asfgit closed this in d6c26d1 Apr 25, 2018
otterc pushed a commit to linkedin/spark that referenced this pull request Mar 22, 2023
Refactor continuous writing to its own class.

See WIP jose-torres#13 for the overall direction this is going, but I think this PR is very isolated and necessary anyway.

existing unit tests - refactoring only

Author: Jose Torres <[email protected]>

Closes apache#21116 from jose-torres/SPARK-24038.

Ref: LIHADOOP-48531

RB=1850759
G=superfriends-reviewers
R=zolin,fli,latang,mshen,yezhou
A=
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.

4 participants