{"action":"created","comment":{"url":"https://api.github.com/repos/twitter/scalding/pulls/comments/86655703","pull_request_review_id":7305950,"id":86655703,"diff_hunk":"@@ -48,24 +52,76 @@ object StatKey {\n }\n \n private[scalding] object CounterImpl {\n-  def apply(fp: FlowProcess[_], statKey: StatKey): CounterImpl =\n-    fp match {\n-      case hFP: HadoopFlowProcess => HadoopFlowPCounterImpl(hFP, statKey)\n-      case _ => GenericFlowPCounterImpl(fp, statKey)\n+  val CounterImplClass = \"scalding.stats.counter.impl.class\"\n+\n+  /*\n+  Note: knowing which counter implementation class to use is a fabric-dependent question. Prior to Cascading 3, the\n+  question of \"which fabric\" was easy: it was either Local or Hadoop, and both types were available wherever Scalding\n+  is loaded.\n+\n+  We cannot have access to the fabric's Mode / ExecutionMode implementation, because instantiating counters happens\n+  at clusterside runtime, not planning time[*]. The FlowProcess[_] detailed type typically also depends on the fabric,\n+  with exceptions: Legacy Hadoop (1.x) and Hadoop2-MR1 use the same FlowProcess implementation class name,\n+  HadoopFlowProcess.\n+  Anyway, we can't use pattern-matching on the specific FlowProcess[_] type, as this would require scalding-core to\n+  know about specific fabric implementation details, which it can no longer do (post-Cascading 3).\n+\n+  Solution found for now:\n+    1. the fabric-specific ExecutionMode is responsible for setting the class name of the appropriate counter\n+    implementation in a config key (named CounterImplClass above)\n+    2. clusterside, CounterImpl$ retrieves this class name and uses reflection to instanciate the correct counter\n+    implementation.\n+\n+\n+  [*] even if Mode was accessible at runtime, it'd require a pervasive addition of (possibly implicit) parameters\n+    whenever a join between pipes is possible, which would cause significant changes to the API\n+\n+   */\n+  def apply(fp: FlowProcess[_], statKey: StatKey): CounterImpl = {\n+    /* TODO: comment this stuff. */\n+    val klassName = Option(fp.getStringProperty(CounterImplClass)).getOrElse(sys.error(CounterImplClass + \" property is missing\"))\n+\n+    val memo = scala.collection.mutable.Map[String, Constructor[CounterImpl]]()\n+    val ctor = memo.synchronized {\n+      memo.getOrElse(klassName,\n+        {\n+          val klass = Class.forName(klassName).asInstanceOf[Class[CounterImpl]]\n+          val ctor = klass.getConstructor(classOf[FlowProcess[_]], classOf[StatKey])\n+          memo.put(klassName, ctor)\n+          ctor\n+        })\n     }\n+    ctor.newInstance(fp, statKey)\n+  }\n+\n+  private[scalding] def upcast[T <: FlowProcess[_]](fp: FlowProcess[_])(implicit ev: TypeTag[T]): T = fp match {\n+    case hfp: T @unchecked if (ev == typeTag[T]) => hfp // see below","path":"scalding-core/src/main/scala/com/twitter/scalding/Stats.scala","position":72,"original_position":72,"commit_id":"5c5127ea61bc1198d6b5151801dc3795ef0c24d3","original_commit_id":"5c5127ea61bc1198d6b5151801dc3795ef0c24d3","user":{"login":"oscar-stripe","id":16548258,"avatar_url":"https://avatars.githubusercontent.com/u/16548258?v=3","gravatar_id":"","url":"https://api.github.com/users/oscar-stripe","html_url":"https://github.com/oscar-stripe","followers_url":"https://api.github.com/users/oscar-stripe/followers","following_url":"https://api.github.com/users/oscar-stripe/following{/other_user}","gists_url":"https://api.github.com/users/oscar-stripe/gists{/gist_id}","starred_url":"https://api.github.com/users/oscar-stripe/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/oscar-stripe/subscriptions","organizations_url":"https://api.github.com/users/oscar-stripe/orgs","repos_url":"https://api.github.com/users/oscar-stripe/repos","events_url":"https://api.github.com/users/oscar-stripe/events{/privacy}","received_events_url":"https://api.github.com/users/oscar-stripe/received_events","type":"User","site_admin":false},"body":"this `typeTag[T]` will always match `ev` no? I think so. Why not just explicitly cast in this method:\r\n```scala\r\ndef downCast[T <: U](u: U): T = u.asInstanceOf[T]\r\n```\r\nAlso, this is a downcast, no?","created_at":"2016-11-05T01:55:34Z","updated_at":"2016-11-05T01:57:44Z","html_url":"https://github.com/twitter/scalding/pull/1617#discussion_r86655703","pull_request_url":"https://api.github.com/repos/twitter/scalding/pulls/1617","_links":{"self":{"href":"https://api.github.com/repos/twitter/scalding/pulls/comments/86655703"},"html":{"href":"https://github.com/twitter/scalding/pull/1617#discussion_r86655703"},"pull_request":{"href":"https://api.github.com/repos/twitter/scalding/pulls/1617"}}},"pull_request":{"url":"https://api.github.com/repos/twitter/scalding/pulls/1617","id":92400941,"html_url":"https://github.com/twitter/scalding/pull/1617","diff_url":"https://github.com/twitter/scalding/pull/1617.diff","patch_url":"https://github.com/twitter/scalding/pull/1617.patch","issue_url":"https://api.github.com/repos/twitter/scalding/issues/1617","number":1617,"state":"open","locked":false,"title":"(🚧🚜 WIP  👷🚧 ) Split fabric-specific code into individual back-ends","user":{"login":"cchepelov","id":1515440,"avatar_url":"https://avatars.githubusercontent.com/u/1515440?v=3","gravatar_id":"","url":"https://api.github.com/users/cchepelov","html_url":"https://github.com/cchepelov","followers_url":"https://api.github.com/users/cchepelov/followers","following_url":"https://api.github.com/users/cchepelov/following{/other_user}","gists_url":"https://api.github.com/users/cchepelov/gists{/gist_id}","starred_url":"https://api.github.com/users/cchepelov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cchepelov/subscriptions","organizations_url":"https://api.github.com/users/cchepelov/orgs","repos_url":"https://api.github.com/users/cchepelov/repos","events_url":"https://api.github.com/users/cchepelov/events{/privacy}","received_events_url":"https://api.github.com/users/cchepelov/received_events","type":"User","site_admin":false},"body":"(as being discussed on scalding-dev)\r\n\r\nIn the things I've left out for now (in addition to the list in the e-mail): scalding-hadoop-test (platform-specific tests on a real minicluster). \r\nPerhaps it'd make sense to have run / runHadoop (→runFabricLocal ?) / runOnMiniCluster in JobTest, and the relevant Mode implementation assortment?\r\n","created_at":"2016-11-04T19:23:15Z","updated_at":"2016-11-05T01:57:44Z","closed_at":null,"merged_at":null,"merge_commit_sha":"578eb4159ecc0ff4cb2beb0e61556f3695b44e66","assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/twitter/scalding/pulls/1617/commits","review_comments_url":"https://api.github.com/repos/twitter/scalding/pulls/1617/comments","review_comment_url":"https://api.github.com/repos/twitter/scalding/pulls/comments{/number}","comments_url":"https://api.github.com/repos/twitter/scalding/issues/1617/comments","statuses_url":"https://api.github.com/repos/twitter/scalding/statuses/5c5127ea61bc1198d6b5151801dc3795ef0c24d3","head":{"label":"cchepelov:split-fabrics","ref":"split-fabrics","sha":"5c5127ea61bc1198d6b5151801dc3795ef0c24d3","user":{"login":"cchepelov","id":1515440,"avatar_url":"https://avatars.githubusercontent.com/u/1515440?v=3","gravatar_id":"","url":"https://api.github.com/users/cchepelov","html_url":"https://github.com/cchepelov","followers_url":"https://api.github.com/users/cchepelov/followers","following_url":"https://api.github.com/users/cchepelov/following{/other_user}","gists_url":"https://api.github.com/users/cchepelov/gists{/gist_id}","starred_url":"https://api.github.com/users/cchepelov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cchepelov/subscriptions","organizations_url":"https://api.github.com/users/cchepelov/orgs","repos_url":"https://api.github.com/users/cchepelov/repos","events_url":"https://api.github.com/users/cchepelov/events{/privacy}","received_events_url":"https://api.github.com/users/cchepelov/received_events","type":"User","site_admin":false},"repo":{"id":31757122,"name":"scalding","full_name":"cchepelov/scalding","owner":{"login":"cchepelov","id":1515440,"avatar_url":"https://avatars.githubusercontent.com/u/1515440?v=3","gravatar_id":"","url":"https://api.github.com/users/cchepelov","html_url":"https://github.com/cchepelov","followers_url":"https://api.github.com/users/cchepelov/followers","following_url":"https://api.github.com/users/cchepelov/following{/other_user}","gists_url":"https://api.github.com/users/cchepelov/gists{/gist_id}","starred_url":"https://api.github.com/users/cchepelov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cchepelov/subscriptions","organizations_url":"https://api.github.com/users/cchepelov/orgs","repos_url":"https://api.github.com/users/cchepelov/repos","events_url":"https://api.github.com/users/cchepelov/events{/privacy}","received_events_url":"https://api.github.com/users/cchepelov/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/cchepelov/scalding","description":"A Scala API for Cascading","fork":true,"url":"https://api.github.com/repos/cchepelov/scalding","forks_url":"https://api.github.com/repos/cchepelov/scalding/forks","keys_url":"https://api.github.com/repos/cchepelov/scalding/keys{/key_id}","collaborators_url":"https://api.github.com/repos/cchepelov/scalding/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/cchepelov/scalding/teams","hooks_url":"https://api.github.com/repos/cchepelov/scalding/hooks","issue_events_url":"https://api.github.com/repos/cchepelov/scalding/issues/events{/number}","events_url":"https://api.github.com/repos/cchepelov/scalding/events","assignees_url":"https://api.github.com/repos/cchepelov/scalding/assignees{/user}","branches_url":"https://api.github.com/repos/cchepelov/scalding/branches{/branch}","tags_url":"https://api.github.com/repos/cchepelov/scalding/tags","blobs_url":"https://api.github.com/repos/cchepelov/scalding/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/cchepelov/scalding/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/cchepelov/scalding/git/refs{/sha}","trees_url":"https://api.github.com/repos/cchepelov/scalding/git/trees{/sha}","statuses_url":"https://api.github.com/repos/cchepelov/scalding/statuses/{sha}","languages_url":"https://api.github.com/repos/cchepelov/scalding/languages","stargazers_url":"https://api.github.com/repos/cchepelov/scalding/stargazers","contributors_url":"https://api.github.com/repos/cchepelov/scalding/contributors","subscribers_url":"https://api.github.com/repos/cchepelov/scalding/subscribers","subscription_url":"https://api.github.com/repos/cchepelov/scalding/subscription","commits_url":"https://api.github.com/repos/cchepelov/scalding/commits{/sha}","git_commits_url":"https://api.github.com/repos/cchepelov/scalding/git/commits{/sha}","comments_url":"https://api.github.com/repos/cchepelov/scalding/comments{/number}","issue_comment_url":"https://api.github.com/repos/cchepelov/scalding/issues/comments{/number}","contents_url":"https://api.github.com/repos/cchepelov/scalding/contents/{+path}","compare_url":"https://api.github.com/repos/cchepelov/scalding/compare/{base}...{head}","merges_url":"https://api.github.com/repos/cchepelov/scalding/merges","archive_url":"https://api.github.com/repos/cchepelov/scalding/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/cchepelov/scalding/downloads","issues_url":"https://api.github.com/repos/cchepelov/scalding/issues{/number}","pulls_url":"https://api.github.com/repos/cchepelov/scalding/pulls{/number}","milestones_url":"https://api.github.com/repos/cchepelov/scalding/milestones{/number}","notifications_url":"https://api.github.com/repos/cchepelov/scalding/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/cchepelov/scalding/labels{/name}","releases_url":"https://api.github.com/repos/cchepelov/scalding/releases{/id}","deployments_url":"https://api.github.com/repos/cchepelov/scalding/deployments","created_at":"2015-03-06T07:10:46Z","updated_at":"2016-10-03T13:19:28Z","pushed_at":"2016-11-04T18:30:35Z","git_url":"git://github.com/cchepelov/scalding.git","ssh_url":"git@github.com:cchepelov/scalding.git","clone_url":"https://github.com/cchepelov/scalding.git","svn_url":"https://github.com/cchepelov/scalding","homepage":"http://twitter.com/scalding","size":14385,"stargazers_count":0,"watchers_count":0,"language":"Scala","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"develop"}},"base":{"label":"twitter:cascading3","ref":"cascading3","sha":"dcc736f77449770ce23c825755337ae4555066d7","user":{"login":"twitter","id":50278,"avatar_url":"https://avatars.githubusercontent.com/u/50278?v=3","gravatar_id":"","url":"https://api.github.com/users/twitter","html_url":"https://github.com/twitter","followers_url":"https://api.github.com/users/twitter/followers","following_url":"https://api.github.com/users/twitter/following{/other_user}","gists_url":"https://api.github.com/users/twitter/gists{/gist_id}","starred_url":"https://api.github.com/users/twitter/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/twitter/subscriptions","organizations_url":"https://api.github.com/users/twitter/orgs","repos_url":"https://api.github.com/users/twitter/repos","events_url":"https://api.github.com/users/twitter/events{/privacy}","received_events_url":"https://api.github.com/users/twitter/received_events","type":"Organization","site_admin":false},"repo":{"id":3146558,"name":"scalding","full_name":"twitter/scalding","owner":{"login":"twitter","id":50278,"avatar_url":"https://avatars.githubusercontent.com/u/50278?v=3","gravatar_id":"","url":"https://api.github.com/users/twitter","html_url":"https://github.com/twitter","followers_url":"https://api.github.com/users/twitter/followers","following_url":"https://api.github.com/users/twitter/following{/other_user}","gists_url":"https://api.github.com/users/twitter/gists{/gist_id}","starred_url":"https://api.github.com/users/twitter/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/twitter/subscriptions","organizations_url":"https://api.github.com/users/twitter/orgs","repos_url":"https://api.github.com/users/twitter/repos","events_url":"https://api.github.com/users/twitter/events{/privacy}","received_events_url":"https://api.github.com/users/twitter/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/twitter/scalding","description":"A Scala API for Cascading","fork":false,"url":"https://api.github.com/repos/twitter/scalding","forks_url":"https://api.github.com/repos/twitter/scalding/forks","keys_url":"https://api.github.com/repos/twitter/scalding/keys{/key_id}","collaborators_url":"https://api.github.com/repos/twitter/scalding/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/twitter/scalding/teams","hooks_url":"https://api.github.com/repos/twitter/scalding/hooks","issue_events_url":"https://api.github.com/repos/twitter/scalding/issues/events{/number}","events_url":"https://api.github.com/repos/twitter/scalding/events","assignees_url":"https://api.github.com/repos/twitter/scalding/assignees{/user}","branches_url":"https://api.github.com/repos/twitter/scalding/branches{/branch}","tags_url":"https://api.github.com/repos/twitter/scalding/tags","blobs_url":"https://api.github.com/repos/twitter/scalding/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/twitter/scalding/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/twitter/scalding/git/refs{/sha}","trees_url":"https://api.github.com/repos/twitter/scalding/git/trees{/sha}","statuses_url":"https://api.github.com/repos/twitter/scalding/statuses/{sha}","languages_url":"https://api.github.com/repos/twitter/scalding/languages","stargazers_url":"https://api.github.com/repos/twitter/scalding/stargazers","contributors_url":"https://api.github.com/repos/twitter/scalding/contributors","subscribers_url":"https://api.github.com/repos/twitter/scalding/subscribers","subscription_url":"https://api.github.com/repos/twitter/scalding/subscription","commits_url":"https://api.github.com/repos/twitter/scalding/commits{/sha}","git_commits_url":"https://api.github.com/repos/twitter/scalding/git/commits{/sha}","comments_url":"https://api.github.com/repos/twitter/scalding/comments{/number}","issue_comment_url":"https://api.github.com/repos/twitter/scalding/issues/comments{/number}","contents_url":"https://api.github.com/repos/twitter/scalding/contents/{+path}","compare_url":"https://api.github.com/repos/twitter/scalding/compare/{base}...{head}","merges_url":"https://api.github.com/repos/twitter/scalding/merges","archive_url":"https://api.github.com/repos/twitter/scalding/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/twitter/scalding/downloads","issues_url":"https://api.github.com/repos/twitter/scalding/issues{/number}","pulls_url":"https://api.github.com/repos/twitter/scalding/pulls{/number}","milestones_url":"https://api.github.com/repos/twitter/scalding/milestones{/number}","notifications_url":"https://api.github.com/repos/twitter/scalding/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/twitter/scalding/labels{/name}","releases_url":"https://api.github.com/repos/twitter/scalding/releases{/id}","deployments_url":"https://api.github.com/repos/twitter/scalding/deployments","created_at":"2012-01-10T16:22:08Z","updated_at":"2016-11-03T16:41:20Z","pushed_at":"2016-11-04T21:36:50Z","git_url":"git://github.com/twitter/scalding.git","ssh_url":"git@github.com:twitter/scalding.git","clone_url":"https://github.com/twitter/scalding.git","svn_url":"https://github.com/twitter/scalding","homepage":"http://twitter.com/scalding","size":22154,"stargazers_count":2662,"watchers_count":2662,"language":"Scala","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":true,"forks_count":613,"mirror_url":null,"open_issues_count":277,"forks":613,"open_issues":277,"watchers":2662,"default_branch":"develop"}},"_links":{"self":{"href":"https://api.github.com/repos/twitter/scalding/pulls/1617"},"html":{"href":"https://github.com/twitter/scalding/pull/1617"},"issue":{"href":"https://api.github.com/repos/twitter/scalding/issues/1617"},"comments":{"href":"https://api.github.com/repos/twitter/scalding/issues/1617/comments"},"review_comments":{"href":"https://api.github.com/repos/twitter/scalding/pulls/1617/comments"},"review_comment":{"href":"https://api.github.com/repos/twitter/scalding/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/twitter/scalding/pulls/1617/commits"},"statuses":{"href":"https://api.github.com/repos/twitter/scalding/statuses/5c5127ea61bc1198d6b5151801dc3795ef0c24d3"}}}}
{"actor":{"display_login":"oscar-stripe"}}