Skip to content

Conversation

@skonto
Copy link
Contributor

@skonto skonto commented Jan 18, 2016

  • Adds support for port constraints on MesosExecutorBackend for fine-grained mode.
    It keeps the convention for random port being the zero port. In such a case, a random port is chosen
    from the available ranges (previously accepted from offers).

Stavros Kontopoulos added 2 commits January 18, 2016 16:40
Conflicts:
	core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendSuite.scala

merge
@skonto skonto force-pushed the honour_ports branch 2 times, most recently from b08a20c to 9991c3f Compare January 18, 2016 17:54
@dragos
Copy link
Contributor

dragos commented Jan 19, 2016

ok to test

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: no need for extra empty lines

@dragos
Copy link
Contributor

dragos commented Jan 19, 2016

a couple of style comments, otherwise this LGTM. @tnachen @andrewor14 can you please have a look?

@SparkQA
Copy link

SparkQA commented Jan 19, 2016

Test build #49685 has finished for PR 10808 at commit 9991c3f.

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

@SparkQA
Copy link

SparkQA commented Jan 20, 2016

Test build #49783 has finished for PR 10808 at commit c4a5115.

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

@SparkQA
Copy link

SparkQA commented Jan 26, 2016

Test build #50100 has finished for PR 10808 at commit 54b18a9.

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

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 used?

@andrewor14
Copy link
Contributor

@dragos @skonto I haven't gone through the entire patch in great detail yet but I have 3 high level concerns:

  1. This patch lacks comments / proper abstractions and makes the code difficult to follow
  2. Currently the only consumer of this port restriction feature is Mesos fine-grained mode, which isn't even the default. I would leave as much logic outside of core's Utils as possible. If we do decide to support it for other modes then we should file a separate issue so we can review that change in isolation.
  3. Fine-grained mode will be deprecated, if not removed, soon. I'm not sure if this change is super worth it given its complexity. If it's a small, isolated change then maybe it makes sense to add it.

I am leaning towards not moving forward with this feature. Even if there is a compelling use case, this feature is slated for removal in a future version anyway so there's not much to gain but a lot to maintain.

@skonto
Copy link
Contributor Author

skonto commented Feb 2, 2016

I tried to do a quick fix here without touching anything other than mesos fine grained mode, my opinion is that the code here needs refactoring in order to be easily extensible.
It is clear that this part is deprecated but when i started working with it, it was not :(. However, i could work on the coarse grain more or less its the same concept.
The few issues could be fixed easily including the abstraction. The real issue is deprecation i think.

@SparkQA
Copy link

SparkQA commented Mar 4, 2016

Test build #52485 has finished for PR 10808 at commit 54b18a9.

  • This patch fails PySpark unit tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@drcrallen
Copy link
Contributor

In https://issues.apache.org/jira/browse/SPARK-15992 I tried to cleanup the offer consideration for the coarse executor a little more. The idea being that using the "improved" refactoring in 15992 would make it easier to add port considerations.

@srowen
Copy link
Member

srowen commented Aug 15, 2016

@skonto you can close this PR now

asfgit pushed a commit that referenced this pull request Aug 15, 2016
…oarse grain mode

- Make mesos coarse grained scheduler accept port offers and pre-assign ports

Previous attempt was for fine grained: #10808

Author: Stavros Kontopoulos <stavros.kontopoulos@lightbend.com>
Author: Stavros Kontopoulos <stavros.kontopoulos@typesafe.com>

Closes #11157 from skonto/honour_ports_coarse.
@skonto
Copy link
Contributor Author

skonto commented Aug 16, 2016

Fixed for coarse grained

@skonto skonto closed this Aug 16, 2016
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.

6 participants