SPARK-11344: Made ApplicationDescription and DriverDescription case classes

[jacek-lewandowski]

DriverDescription refactored to case class because it included no mutable fields.

ApplicationDescription had one mutable field, which was appUiUrl. This field was set by the driver to point to the driver web UI. Master was modifying this field when the application was removed to redirect requests to history server. This was wrong because objects which are sent over the wire should be immutable. Now appUiUrl is immutable in ApplicationDescription and always points to the driver UI even if it is already shutdown. The UI url which master exposes to the user and modifies dynamically is now included into ApplicationInfo - a data object which describes the application state internally in master. That URL in ApplicationInfo is initialised with the value from ApplicationDescription.

ApplicationDescription also included value user, which is now a part of case class fields.

[srowen]

I'm not an expert here but I tend to like this kind of change.

[SparkQA]

Test build #44427 has finished for PR 9299 at commit 1870a5a.

• This patch fails to build.
• This patch merges cleanly.
• This patch adds no public classes.

[jacek-lewandowski]

Thanks @srowen

[SparkQA]

Test build #44441 has finished for PR 9299 at commit 4eb1f97.

• This patch fails to build.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #44446 has finished for PR 9299 at commit 169c053.

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

[jacek-lewandowski]

@JoshRosen could you verify this?

[srowen]

This is the only possible downside, that you only moved the mutable field to another class. Still, I think it is a reasonable update. @JoshRosen ?

[jacek-lewandowski]

Yes, I moved it to that class because it represent a mutable state of application in the Master - and if the appUiUrl changes, it is due to application state change. This class has many more mutable fields so it doesn't make it worse. The main purpose of this ticket was to move out mutable fields from ApplicationDescription which is used to transfer data from one process to another and by definition it should be immutable.

[srowen]

CC @andrewor14 for a look but I think this is an OK small refactoring.

[srowen]

@jacek-lewandowski on one final re-read my last question is -- why does ApplicationDescription still have appUiUrl then? in several cases it's just copied into an ApplicationInfo that contains it. You have an 'original' value in the description and some possibly changed copy in the info object. Is that the intent? can it move out of the description class entirely?

[jacek-lewandowski]

@srowen That is the intent. I think I cannot remove it from ApplicationDescription because this address is set by the driver. Then, the driver put it into the ApplicationDescription object and send to Master. If I removed it, Master wouldn't learn about the UI address of the driver.

[srowen]

OK then the value in ApplicationDescription is just the initial value? maybe we can rename this field to indicate that it's only the configured or initial value in some way, to avoid confusion. Then I think this is pretty good.

[jacek-lewandowski]

@srowen I can rename it if you insist. However, this value is in fact immutable. The driver UI address does not get changed. Master replaces URLs which points to driver's UI when the driver is dead, but this doesn't mean the driver UI address was changed. It just became unavailable. So, imo the current names are ok. Though, if you think they are confusing, it would be better to change the name in ApplicationInfo to something like appUIOrNotFound or appUIPlaceholder.

[srowen]

I don't think the fact that one of the values is immutable matters. There are two copies of the same value within the same object (in ApplicationInfo and in the ApplicationDescription inside it) one of which never changes and one of which changes. Both have the same name. They may diverge in value (right?) This seems confusing. Can we do anything with naming or comments to clarify?

So maybe ApplicationInfo should indeed rename its field to ... yes, appUIOrPlaceholderURI or something? Maybe it can be decomposed further: ApplicationInfo can have a "placeholder" or "override" Option[String] field whose value is returned if it exists or else the value from ApplicationDescription is returned? Maybe I'm over-thinking that. But it seems like we can add a little more clarity here to avoid, say, someone later accessing ApplicationInfo.ApplicationDescription.appUiUri and not realizing it's not the current, possibly-overridden value like it used to be.

[jacek-lewandowski]

Ok, agree

[SparkQA]

Test build #44803 has finished for PR 9299 at commit 3c6b529.

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

[jacek-lewandowski]

@srowen ^

[srowen]

Looking good to me. Let me leave it a bit for other input.

[srowen]

merged to master