[BEAM-248] Add display data to additional PTransforms #280
Conversation
swegner
force-pushed
the
displaydata-leaves
branch
3 times, most recently
from
c25ddaa to
cd77d1b
Compare
|
R: @bjchambers |
|
|
||
| private static final Pattern HAS_GLOB = Pattern.compile(".*(\\[|\\*|\\?).*"); | ||
| public String getBrowseUrl() { | ||
| StringBuilder builder = new StringBuilder("https://console.cloud.google.com/storage/browser/"); |
There was a problem hiding this comment.
It appears these URLs work for browsing to "buckets" (i.e. directories), but not "objects" (i.e. files). There's another URL schema, "https://storage.cloud.google.com/*" which works for exactly the opposite-- buckets but not objects. Without doing a query, we won't always be able to tell what's a bucket and what's an object.
There was a problem hiding this comment.
There was a problem hiding this comment.
I've split off link URLs to a separate PR: #300
swegner
force-pushed
the
displaydata-leaves
branch
from
cd77d1b to
1dccf39
Compare
|
I've addressed all feedback so far. Please take another look. @bjchambers |
| * | ||
| * @param tempLocation Temp location for {@link DataflowPipelineOptions#setTempLocation(String)} | ||
| */ | ||
| static DisplayDataEvaluator create(File tempLocation) { |
There was a problem hiding this comment.
If this is only used for testing, I'd propose taking a TemporaryFile instance directly. That will force the tests to be properly written to use @Rule TemporaryFile tempFile and then call DisplayDataEvaluator.create(tempFile). Other option which would be even more extreme is to make this a @Rule.
There was a problem hiding this comment.
The temp value parameter is gone in the latest version. Some DataflowPipelineOptions require a GCS path, so we're using a stub GCS instead.
swegner
force-pushed
the
displaydata-leaves
branch
2 times, most recently
from
1e08c6d to
664536a
Compare
|
I've addressed all feedback so far. Please take another look. @bjchambers |
| } | ||
|
|
||
| @Test | ||
| public void testStreamingSinkPrimitiveDisplayData() { |
There was a problem hiding this comment.
I think for Junit4 it is preferred not to prefix with "test". So just"
@Test
public void streamingSinkPrimitiveDisplayData
etc.
There was a problem hiding this comment.
The convention is split in the BEAM codebase, but most test cases use the "test" prefix: 1916 prefixed vs 93 not according to this script.
I'm going to follow the BEAM convention. It makes sense to migrate if we're not following JUnit4 convention, but we should do it across the codebase. Feel free to file a bug assigned to me if you feel it's worth the effort.
|
Some comments, then LGTM. |
swegner
force-pushed
the
displaydata-leaves
branch
from
2561b81 to
d82d1a5
Compare
|
I've addressed all feedback so far. Please take another look. @bjchambers |
|
LGTM |
DisplayDataEvaluator is useful for validating how PTransform display data is surfaced in the context of a Pipeline and runner.
swegner
force-pushed
the
displaydata-leaves
branch
from
d82d1a5 to
3ea62f8
Compare
|
Backport: GoogleCloudPlatform/DataflowJavaSDK#230 |
…uire field data (apache#280) * fix: limit data transferred as part of list_documents as we don't require field data
3 participants
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
<Jira issue #>in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.