Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Refactor duplicated code in SubmitterMain and SchedulerConfig #1599

Merged
merged 1 commit into from
Dec 8, 2016
Merged

Refactor duplicated code in SubmitterMain and SchedulerConfig #1599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions heron/common/src/java/com/twitter/heron/common/basics/FileUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,17 @@ public static boolean isOriginalPackagePex(String packageFilename) {
public static String getBaseName(String file) {
return new File(file).getName();
}

public static String getPkgType(String topologyBinaryFile) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the removal of dup code!

Could you also please change to not abbreviate pkg (e.g., packageType and getPackageType).

Also instead of returning strings, better to use an enum, maybe as an inner enum in FileUtils:

enum PackageType {
  PEX,
  JAR,
  TAR
}

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for merging this too fast. I'll do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, returning enum might not be what we want, since we need to put value as string into config. See: https://github.com/twitter/heron/pull/1599/files#diff-1043de1ec8f8e4c0194d4e6035e654cfL80

Copy link
Contributor

Choose a reason for hiding this comment

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

Config.put takes an object as it's value, so we could put enum values in and add a getter that returns it. We did this with ByteAmount for example:

https://github.com/twitter/heron/blob/master/heron/spi/src/java/com/twitter/heron/spi/common/Config.java#L79

Then we refactor all consumers to use the enum.

The only catch is that if we add the helper to Config, then the enum needs to live somewhere central like in spi. This might not be a bad thing though. It's much better to have a strong set of package types than to pass strings around...

Copy link
Contributor

Choose a reason for hiding this comment

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

where do you think is appropriate to put this? I am really bad at finding an appropriate class to add code😔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@objmagic - hi, Is there anything I could do? The method name which @billonahill mentioned is better . However, I don't think adding an new enum class is a good idea. Perhaps we could add new static constants in FileUtils instead of handling string variable explicitly, for example

private static final String PEX = "pex";

Copy link
Contributor

@objmagic objmagic Dec 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@mycFelix any reason why using enum is not a good idea?

Copy link
Contributor Author

@mycFelix mycFelix Dec 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@objmagic - Cause String pkgType is used for Config's value. Just in my opion, this scene is applicable to the principle of Occam's razor -- If not necessary, do not increase the entity

Copy link
Contributor

Choose a reason for hiding this comment

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

Config supports objects and we should favor type safety where ever possible. Doing so will make code easier to maintain and less prone to bugs than by passing string values around.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mycFelix imo Bill has better arguments. I will submit a PR to use enum.

String pkgType;
String basename = FileUtils.getBaseName(topologyBinaryFile);
if (FileUtils.isOriginalPackagePex(basename)) {
pkgType = "pex";
} else if (FileUtils.isOriginalPackageJar(basename)) {
pkgType = "jar";
} else {
pkgType = "tar";
}
return pkgType;
}
}
11 changes: 1 addition & 10 deletions heron/scheduler-core/src/java/com/twitter/heron/scheduler/SubmitterMain.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,7 @@ public class SubmitterMain {
protected static Config topologyConfigs(
String topologyPackage, String topologyBinaryFile, String topologyDefnFile,
TopologyAPI.Topology topology) {

String pkgType;
String basename = FileUtils.getBaseName(topologyBinaryFile);
if (FileUtils.isOriginalPackagePex(basename)) {
pkgType = "pex";
} else if (FileUtils.isOriginalPackageJar(basename)) {
pkgType = "jar";
} else {
pkgType = "tar";
}
String pkgType = FileUtils.getPkgType(topologyBinaryFile);

Config config = Config.newBuilder()
.put(Keys.topologyId(), topology.getId())
Expand Down
10 changes: 1 addition & 9 deletions heron/spi/src/java/com/twitter/heron/spi/utils/SchedulerConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,7 @@ private SchedulerConfig() {
*/
protected static Config topologyConfigs(String topologyBinaryFile,
String topologyDefnFile, TopologyAPI.Topology topology) {
String basename = FileUtils.getBaseName(topologyBinaryFile);
String pkgType;
if (FileUtils.isOriginalPackagePex(basename)) {
pkgType = "pex";
} else if (FileUtils.isOriginalPackageJar(basename)) {
pkgType = "jar";
} else {
pkgType = "tar";
}
String pkgType = FileUtils.getPkgType(topologyBinaryFile);

Config config = Config.newBuilder()
.put(Keys.topologyId(), topology.getId())
Expand Down