You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nutch.apache.org by GitBox <gi...@apache.org> on 2020/06/09 16:46:59 UTC
[GitHub] [nutch] pmezard opened a new pull request #533: NUTCH-2791 Handle GCS URLs in stats commands
pmezard opened a new pull request #533:
URL: https://github.com/apache/nutch/pull/533
- Handle Google Cloud Storage URLs as crawldb inputs in domainstats,
protocolstats and crawlcomplete commands.
- Correctly resolve numReducers in protocolstats.
- Align crawlcomplete -inputDirs behaviour on the other commands: expect
directories containing "current", not "crawldb/current".
Like the other PR, I cannot run all tests locally. But I am not too worried, test-core pass.
Also, these changes are not super high quality in the sense that ideally I would have added some helper to enforce path manipulations which work with URLs, for all commands. Now, I do not think these commands deserve so much work so:
- I just copy/paste "Path" manipulations used in other commands like CrawlDBReader which I know work locally and in GCS. Then run the commands in GCP and some of them again locally just to be sure I did not break everything. I do not know Java API well enough to decide if the path manipulations make sense, I just know they work.
- I spotted the numReducers issue while reading the code. I did not test it. It is obviously a copy/paste from domainstats to protocolstats.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nutch] sebastian-nagel merged pull request #533: NUTCH-2791 Handle GCS URLs in stats commands
Posted by GitBox <gi...@apache.org>.
sebastian-nagel merged pull request #533:
URL: https://github.com/apache/nutch/pull/533
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nutch] sebastian-nagel commented on a change in pull request #533: NUTCH-2791 Handle GCS URLs in stats commands
Posted by GitBox <gi...@apache.org>.
sebastian-nagel commented on a change in pull request #533:
URL: https://github.com/apache/nutch/pull/533#discussion_r438319215
##########
File path: src/java/org/apache/nutch/util/CrawlCompletionStats.java
##########
@@ -153,9 +153,7 @@ public int run(String[] args) throws Exception {
String[] inputDirsSpecs = inputDir.split(",");
for (int i = 0; i < inputDirsSpecs.length; i++) {
- File completeInputPath = new File(new File(inputDirsSpecs[i]), "crawldb/current");
- FileInputFormat.addInputPath(job, new Path(completeInputPath.toString()));
-
+ FileInputFormat.addInputPath(job, new Path(inputDirsSpecs[i], "current"));
Review comment:
This would require to change the command-line help as well:
```
$> bin/nutch crawlcomplete
usage: CrawlCompletionStats [-h] -inputDirs <inputDirs> -mode <mode>
[-numReducers <numReducers>] -outputDir <outputDir>
-h,--help Show this message
-inputDirs <inputDirs> Comma separated list of crawl directories
(e.g., "./crawl1,./crawl2")
...
```
This changes the behavior of the completion stats. But I agree with the change, we cannot assume that the name of the crawldb is fix - although this is the name used by the script "bin/crawl".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nutch] sebastian-nagel commented on pull request #533: NUTCH-2791 Handle GCS URLs in stats commands
Posted by GitBox <gi...@apache.org>.
sebastian-nagel commented on pull request #533:
URL: https://github.com/apache/nutch/pull/533#issuecomment-642579892
> do you want them squashed immediately
> As a reviewer I prefer independant commits to avoir having to re-read everything but the README suggests you prefer to squash everything.
I totally agree. It doesn't really matter if the changes are short (as here). But if it's about hundred or more lines of code, separate commits (each with a [meaningful commit message](http://www.commitstrip.com/en/2017/09/12/versioning-is-important/)) are better. Same applies if a larger contribution includes automatic code reformats or refactorings (usually no need to review them, at least, not carefully).
> What is the correct way to test such a change to nutch script help? I ran ant zip-bin and redeployed the code locally but maybe there is something faster, working directly in-tree?
1. compile Nutch and build the runtime: `ant runtime` or just `ant`
2. then call `runtime/local/bin/nutch` (have a look at the environment variables in `bin/nutch` which allow to override the log folder or directories where config files are loaded from)
Testing of distributed mode is naturally much more complex (eg. see [here](/sebastian-nagel/nutch-test-single-node-cluster))
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nutch] pmezard commented on pull request #533: NUTCH-2791 Handle GCS URLs in stats commands
Posted by GitBox <gi...@apache.org>.
pmezard commented on pull request #533:
URL: https://github.com/apache/nutch/pull/533#issuecomment-642450747
Fixed and rebased, PTAL.
Two questions:
- When you request changes, do you want them squashed immediately in the new code or do you prefer independant commit. As a reviewer I prefer independant commits to avoir having to re-read everything but the README suggests you prefer to squash everything.
- What is the correct way to test such a change to nutch script help? I ran `ant zip-bin` and redeployed the code locally but maybe there is something faster, working directly in-tree?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org