From 782a2f5b48d54dd839398b14b72c878f7b526a10 Mon Sep 17 00:00:00 2001 From: Antonin Delpeuch Date: Tue, 7 Mar 2017 20:21:27 +0000 Subject: [PATCH 1/5] Add caching in URL fetching --- ...ColumnAdditionByFetchingURLsOperation.java | 187 ++++++++++-------- 1 file changed, 105 insertions(+), 82 deletions(-) diff --git a/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java b/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java index 3160a0153..ebcb4fcbd 100644 --- a/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java +++ b/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java @@ -23,8 +23,8 @@ LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT -LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, -DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. @@ -43,6 +43,8 @@ import java.net.URLConnection; import java.util.ArrayList; import java.util.List; import java.util.Properties; +import java.util.Map; +import java.util.HashMap; import org.json.JSONException; import org.json.JSONObject; @@ -76,14 +78,14 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat final protected String _baseColumnName; final protected String _urlExpression; final protected OnError _onError; - + final protected String _newColumnName; final protected int _columnInsertIndex; final protected int _delay; static public AbstractOperation reconstruct(Project project, JSONObject obj) throws Exception { JSONObject engineConfig = obj.getJSONObject("engineConfig"); - + return new ColumnAdditionByFetchingURLsOperation( engineConfig, obj.getString("baseColumnName"), @@ -94,32 +96,32 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat obj.getInt("delay") ); } - + public ColumnAdditionByFetchingURLsOperation( JSONObject engineConfig, String baseColumnName, String urlExpression, OnError onError, - String newColumnName, + String newColumnName, int columnInsertIndex, int delay ) { super(engineConfig); - + _baseColumnName = baseColumnName; _urlExpression = urlExpression; _onError = onError; - + _newColumnName = newColumnName; _columnInsertIndex = columnInsertIndex; - + _delay = delay; } @Override public void write(JSONWriter writer, Properties options) throws JSONException { - + writer.object(); writer.key("op"); writer.value(OperationRegistry.s_opClassToName.get(this.getClass())); writer.key("description"); writer.value(getBriefDescription(null)); @@ -135,44 +137,45 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat @Override protected String getBriefDescription(Project project) { - return "Create column " + _newColumnName + - " at index " + _columnInsertIndex + - " by fetching URLs based on column " + _baseColumnName + + return "Create column " + _newColumnName + + " at index " + _columnInsertIndex + + " by fetching URLs based on column " + _baseColumnName + " using expression " + _urlExpression; } protected String createDescription(Column column, List cellsAtRows) { - return "Create new column " + _newColumnName + + return "Create new column " + _newColumnName + ", filling " + cellsAtRows.size() + - " rows by fetching URLs based on column " + column.getName() + + " rows by fetching URLs based on column " + column.getName() + " and formulated as " + _urlExpression; } - - + + @Override public Process createProcess(Project project, Properties options) throws Exception { Engine engine = createEngine(project); engine.initializeFromJSON(_engineConfig); - + Evaluable eval = MetaParser.parse(_urlExpression); - + return new ColumnAdditionByFetchingURLsProcess( - project, + project, engine, eval, getBriefDescription(null) ); } - + public class ColumnAdditionByFetchingURLsProcess extends LongRunningProcess implements Runnable { - final protected Project _project; - final protected Engine _engine; - final protected Evaluable _eval; - final protected long _historyEntryID; - protected int _cellIndex; + final protected Project _project; + final protected Engine _engine; + final protected Evaluable _eval; + final protected long _historyEntryID; + protected int _cellIndex; + protected Map _urlCache; public ColumnAdditionByFetchingURLsProcess( - Project project, + Project project, Engine engine, Evaluable eval, String description @@ -182,12 +185,13 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat _engine = engine; _eval = eval; _historyEntryID = HistoryEntry.allocateID(); + _urlCache = new HashMap(); } - + @Override public void write(JSONWriter writer, Properties options) throws JSONException { - + writer.object(); writer.key("id"); writer.value(hashCode()); writer.key("description"); writer.value(_description); @@ -196,12 +200,12 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat writer.key("progress"); writer.value(_progress); writer.endObject(); } - + @Override protected Runnable getRunnable() { return this; } - + @Override public void run() { Column column = _project.columnModel.getColumnByName(_baseColumnName); @@ -213,66 +217,89 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat _project.processManager.onFailedProcess(this, new Exception("Another column already named " + _newColumnName)); return; } - + List urls = new ArrayList(_project.rows.size()); - + FilteredRows filteredRows = _engine.getAllFilteredRows(); filteredRows.accept(_project, createRowVisitor(urls)); - + List responseBodies = new ArrayList(urls.size()); for (int i = 0; i < urls.size(); i++) { CellAtRow urlData = urls.get(i); - long start = System.currentTimeMillis(); - CellAtRow cellAtRow = fetch(urlData); + CellAtRow cellAtRow = cachedFetch(urlData); if (cellAtRow != null) { responseBodies.add(cellAtRow); } - + _progress = i * 100 / urls.size(); - try { - long delay = _delay - (System.currentTimeMillis() - start); - if (delay > 0) { - Thread.sleep(delay); - } - } catch (InterruptedException e) { - if (_canceled) { - break; - } - } + + if (_canceled) { + break; + } } - + + _urlCache.clear(); + if (!_canceled) { - HistoryEntry historyEntry = new HistoryEntry( _historyEntryID, - _project, - _description, - ColumnAdditionByFetchingURLsOperation.this, + _project, + _description, + ColumnAdditionByFetchingURLsOperation.this, new ColumnAdditionChange( _newColumnName, _columnInsertIndex, responseBodies) ); - + _project.history.addEntry(historyEntry); _project.processManager.onDoneProcess(this); } } - - CellAtRow fetch(CellAtRow urlData) { - String urlString = urlData.cell.value.toString(); - URL url = null; - - try { - url = new URL(urlString); - } catch (MalformedURLException e) { - return null; - } - + + CellAtRow cachedFetch(CellAtRow urlData) { + String urlString = urlData.cell.value.toString(); + URL url = null; + try { + url = new URL(urlString); + } catch (MalformedURLException e) { + return null; + } + + Serializable cellResult = _urlCache.get(url); + if (cellResult == null) { + cellResult = fetch(url); + if (cellResult != null) { + _urlCache.put(url, cellResult); + } + + try { + // Always sleep for the delay, no matter how long the + // request took. This is more responsible than substracting + // the time spend requesting the URL, because it naturally + // slows us down if the server is busy and takes a long time + // to reply. + if (_delay > 0) { + Thread.sleep(_delay); + } + } catch (InterruptedException e) { + return null; + } + } + + if (cellResult != null) { + return new CellAtRow( + urlData.row, + new Cell(cellResult, null)); + } + return null; + } + + Serializable fetch(URL url) { try { URLConnection urlConnection = url.openConnection(); // urlConnection.setRequestProperty(_headerKey, _headerValue); - + try { InputStream is = urlConnection.getInputStream(); try { @@ -287,12 +314,8 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat } } } - return new CellAtRow( - urlData.row, - new Cell( - ParsingUtilities.inputStreamToString( - is, (encoding == null) || ( encoding.equalsIgnoreCase("\"UTF-8\"")) ? "UTF-8" : encoding), - null)); + return ParsingUtilities.inputStreamToString( + is, (encoding == null) || ( encoding.equalsIgnoreCase("\"UTF-8\"")) ? "UTF-8" : encoding); } finally { is.close(); @@ -313,11 +336,11 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat message = e.toString(); } return _onError == OnError.StoreError ? - new CellAtRow(urlData.row, new Cell(new EvalError(message), null)) : null; + new EvalError(message) : null; } } catch (Exception e) { return _onError == OnError.StoreError ? - new CellAtRow(urlData.row, new Cell(new EvalError(e.getMessage()), null)) : null; + new EvalError(e.getMessage()) : null; } } @@ -326,33 +349,33 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat int cellIndex; Properties bindings; List cellsAtRows; - + public RowVisitor init(List cellsAtRows) { Column column = _project.columnModel.getColumnByName(_baseColumnName); - + this.cellIndex = column.getCellIndex(); this.bindings = ExpressionUtils.createBindings(_project); this.cellsAtRows = cellsAtRows; return this; } - + @Override public void start(Project project) { // nothing to do } - + @Override public void end(Project project) { // nothing to do } - + @Override public boolean visit(Project project, int rowIndex, Row row) { Cell cell = row.getCell(cellIndex); Cell newCell = null; - + ExpressionUtils.bind(bindings, row, rowIndex, _baseColumnName, cell); - + Object o = _eval.evaluate(bindings); if (o != null) { if (o instanceof Cell) { @@ -366,11 +389,11 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat } } } - + if (newCell != null) { cellsAtRows.add(new CellAtRow(rowIndex, newCell)); } - + return false; } }.init(cellsAtRows); From 3297e6e7eeafa160874347dfe18a6f3897c1b7d3 Mon Sep 17 00:00:00 2001 From: Antonin Delpeuch Date: Wed, 8 Mar 2017 00:47:17 +0000 Subject: [PATCH 2/5] Add tests for the URL caching feature --- .../refine/tests/model/UrlFetchingTests.java | 169 ++++++++++++++++++ 1 file changed, 169 insertions(+) create mode 100644 main/tests/server/src/com/google/refine/tests/model/UrlFetchingTests.java diff --git a/main/tests/server/src/com/google/refine/tests/model/UrlFetchingTests.java b/main/tests/server/src/com/google/refine/tests/model/UrlFetchingTests.java new file mode 100644 index 000000000..1236fb71d --- /dev/null +++ b/main/tests/server/src/com/google/refine/tests/model/UrlFetchingTests.java @@ -0,0 +1,169 @@ +/* + +Copyright 2010, Google Inc. +All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are +met: + + * Redistributions of source code must retain the above copyright +notice, this list of conditions and the following disclaimer. + * Redistributions in binary form must reproduce the above +copyright notice, this list of conditions and the following disclaimer +in the documentation and/or other materials provided with the +distribution. + * Neither the name of Google Inc. nor the names of its +contributors may be used to endorse or promote products derived from +this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +*/ + +package com.google.refine.tests.model; + +import static org.mockito.Mockito.mock; + +import java.io.File; +import java.io.IOException; +import java.util.Properties; + +import org.json.JSONException; +import org.json.JSONObject; +import org.slf4j.LoggerFactory; +import org.testng.Assert; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.BeforeTest; +import org.testng.annotations.Test; + +import com.google.refine.ProjectManager; +import com.google.refine.ProjectMetadata; +import com.google.refine.browsing.Engine; +import com.google.refine.browsing.RowVisitor; +import com.google.refine.grel.Function; +import com.google.refine.io.FileProjectManager; +import com.google.refine.model.Cell; +import com.google.refine.model.Column; +import com.google.refine.model.ModelException; +import com.google.refine.model.Project; +import com.google.refine.model.Row; +import com.google.refine.process.Process; +import com.google.refine.process.ProcessManager; +import com.google.refine.operations.OnError; +import com.google.refine.operations.EngineDependentOperation; +import com.google.refine.operations.column.ColumnAdditionByFetchingURLsOperation; +import com.google.refine.tests.RefineTest; +import com.google.refine.tests.util.TestUtils; + + +public class UrlFetchingTests extends RefineTest { + + static final String ENGINE_JSON_URLS = "{\"mode\":\"row-based\"}}"; + + @Override + @BeforeTest + public void init() { + logger = LoggerFactory.getLogger(this.getClass()); + } + + // dependencies + Project project; + Properties options; + JSONObject engine_config; + Engine engine; + Properties bindings; + + @BeforeMethod + public void SetUp() throws JSONException, IOException, ModelException { + File dir = TestUtils.createTempDirectory("openrefine-test-workspace-dir"); + FileProjectManager.initialize(dir); + project = new Project(); + ProjectMetadata pm = new ProjectMetadata(); + pm.setName("URL Fetching Test Project"); + ProjectManager.singleton.registerProject(project, pm); + + int index = project.columnModel.allocateNewCellIndex(); + Column column = new Column(index,"fruits"); + project.columnModel.addColumn(index, column, true); + + options = mock(Properties.class); + engine = new Engine(project); + engine_config = new JSONObject(ENGINE_JSON_URLS); + engine.initializeFromJSON(engine_config); + engine.setMode(Engine.Mode.RowBased); + + bindings = new Properties(); + bindings.put("project", project); + + } + + @AfterMethod + public void TearDown() { + project = null; + options = null; + engine = null; + bindings = null; + } + + /** + * Test for caching + */ + + @Test + public void testUrlCaching() throws Exception { + for (int i = 0; i < 100; i++) { + Row row = new Row(2); + row.setCell(0, new Cell(i < 5 ? "apple":"orange", null)); + project.rows.add(row); + } + //engine.getAllRows().accept(project, new CountingRowVisitor(5)) ; + //engine.getAllFilteredRows().accept(project, new CountingRowVisitor(4)); + // Function fc = new FacetCount(); + // Integer count = (Integer) fc.call(bindings, new Object[] {"a", "value", "Column A"}); + // Assert.assertEquals(count.intValue(), 4); + // EngineDependentOperation op = new RowRemovalOperation(engine_config); + EngineDependentOperation op = new ColumnAdditionByFetchingURLsOperation(engine_config, + "fruits", + "\"https://www.random.org/integers/?num=1&min=1&max=100&col=1&base=10&format=plain&rnd=new&city=\"+value", + OnError.SetToBlank, + "rand", + 1, + 500); + ProcessManager pm = project.getProcessManager(); + Process process = op.createProcess(project, options); + process.startPerforming(pm); + Assert.assertTrue(process.isRunning()); + try { + // We have 100 rows and 500 ms per row but only two distinct + // values so we should not wait more than ~2000 ms to get the + // results. Just to make sure the test passes with plenty of + // net latency we sleep for longer (but still less than + // 50,000ms). + Thread.sleep(5000); + } catch (InterruptedException e) { + Assert.fail("Test interrupted"); + } + Assert.assertFalse(process.isRunning()); + + // Inspect rows + String ref_val = (String)project.rows.get(0).getCellValue(1); + Assert.assertTrue(ref_val != "apple"); // just to make sure I picked the right column + for (int i = 1; i < 4; i++) { + // all random values should be equal due to caching + Assert.assertEquals(project.rows.get(i).getCellValue(1), ref_val); + } + } + +} From a9c4b0af16ac702c30ddfb229c16b2bb1722d80f Mon Sep 17 00:00:00 2001 From: Antonin Delpeuch Date: Wed, 8 Mar 2017 07:45:11 +0000 Subject: [PATCH 3/5] Cache String, not URL, in ColumnAdditionByFetchingURLsOperation --- ...ColumnAdditionByFetchingURLsOperation.java | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java b/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java index ebcb4fcbd..d29b85d52 100644 --- a/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java +++ b/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java @@ -172,7 +172,7 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat final protected Evaluable _eval; final protected long _historyEntryID; protected int _cellIndex; - protected Map _urlCache; + protected Map _urlCache; public ColumnAdditionByFetchingURLsProcess( Project project, @@ -185,7 +185,7 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat _engine = engine; _eval = eval; _historyEntryID = HistoryEntry.allocateID(); - _urlCache = new HashMap(); + _urlCache = new HashMap(); } @Override @@ -259,18 +259,12 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat CellAtRow cachedFetch(CellAtRow urlData) { String urlString = urlData.cell.value.toString(); - URL url = null; - try { - url = new URL(urlString); - } catch (MalformedURLException e) { - return null; - } - Serializable cellResult = _urlCache.get(url); + Serializable cellResult = _urlCache.get(urlString); if (cellResult == null) { - cellResult = fetch(url); + cellResult = fetch(urlString); if (cellResult != null) { - _urlCache.put(url, cellResult); + _urlCache.put(urlString, cellResult); } try { @@ -295,7 +289,14 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat return null; } - Serializable fetch(URL url) { + Serializable fetch(String urlString) { + URL url = null; + try { + url = new URL(urlString); + } catch (MalformedURLException e) { + return null; + } + try { URLConnection urlConnection = url.openConnection(); // urlConnection.setRequestProperty(_headerKey, _headerValue); From 32c232c2d64927f8ec803ac227541e1ee64f8871 Mon Sep 17 00:00:00 2001 From: Antonin Delpeuch Date: Wed, 8 Mar 2017 09:32:34 +0000 Subject: [PATCH 4/5] Move to Guava's cache for ColumnAdditionByFetchingURLsOperation --- ...ColumnAdditionByFetchingURLsOperation.java | 57 +++++++++++-------- .../refine/tests/model/UrlFetchingTests.java | 6 -- 2 files changed, 32 insertions(+), 31 deletions(-) diff --git a/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java b/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java index d29b85d52..445383101 100644 --- a/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java +++ b/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java @@ -43,8 +43,8 @@ import java.net.URLConnection; import java.util.ArrayList; import java.util.List; import java.util.Properties; -import java.util.Map; -import java.util.HashMap; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.ExecutionException; import org.json.JSONException; import org.json.JSONObject; @@ -73,6 +73,9 @@ import com.google.refine.operations.cell.TextTransformOperation; import com.google.refine.process.LongRunningProcess; import com.google.refine.process.Process; import com.google.refine.util.ParsingUtilities; +import com.google.common.cache.CacheBuilder; +import com.google.common.cache.LoadingCache; +import com.google.common.cache.CacheLoader; public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperation { final protected String _baseColumnName; @@ -172,7 +175,7 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat final protected Evaluable _eval; final protected long _historyEntryID; protected int _cellIndex; - protected Map _urlCache; + protected LoadingCache _urlCache; public ColumnAdditionByFetchingURLsProcess( Project project, @@ -185,7 +188,28 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat _engine = engine; _eval = eval; _historyEntryID = HistoryEntry.allocateID(); - _urlCache = new HashMap(); + _urlCache = CacheBuilder.newBuilder() + .maximumSize(2048) + .expireAfterWrite(10, TimeUnit.MINUTES) + .build( + new CacheLoader() { + public Serializable load(String urlString) { + Serializable result = fetch(urlString); + try { + // Always sleep for the delay, no matter how long the + // request took. This is more responsible than substracting + // the time spend requesting the URL, because it naturally + // slows us down if the server is busy and takes a long time + // to reply. + if (_delay > 0) { + Thread.sleep(_delay); + } + } catch (InterruptedException e) { + return null; + } + return result; + } + }); } @Override @@ -238,8 +262,6 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat } } - _urlCache.clear(); - if (!_canceled) { HistoryEntry historyEntry = new HistoryEntry( _historyEntryID, @@ -260,25 +282,10 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat CellAtRow cachedFetch(CellAtRow urlData) { String urlString = urlData.cell.value.toString(); - Serializable cellResult = _urlCache.get(urlString); - if (cellResult == null) { - cellResult = fetch(urlString); - if (cellResult != null) { - _urlCache.put(urlString, cellResult); - } - - try { - // Always sleep for the delay, no matter how long the - // request took. This is more responsible than substracting - // the time spend requesting the URL, because it naturally - // slows us down if the server is busy and takes a long time - // to reply. - if (_delay > 0) { - Thread.sleep(_delay); - } - } catch (InterruptedException e) { - return null; - } + Serializable cellResult = null; + try { + cellResult = _urlCache.get(urlString); + } catch(ExecutionException e) { } if (cellResult != null) { diff --git a/main/tests/server/src/com/google/refine/tests/model/UrlFetchingTests.java b/main/tests/server/src/com/google/refine/tests/model/UrlFetchingTests.java index 1236fb71d..02f2b7e8b 100644 --- a/main/tests/server/src/com/google/refine/tests/model/UrlFetchingTests.java +++ b/main/tests/server/src/com/google/refine/tests/model/UrlFetchingTests.java @@ -128,12 +128,6 @@ public class UrlFetchingTests extends RefineTest { row.setCell(0, new Cell(i < 5 ? "apple":"orange", null)); project.rows.add(row); } - //engine.getAllRows().accept(project, new CountingRowVisitor(5)) ; - //engine.getAllFilteredRows().accept(project, new CountingRowVisitor(4)); - // Function fc = new FacetCount(); - // Integer count = (Integer) fc.call(bindings, new Object[] {"a", "value", "Column A"}); - // Assert.assertEquals(count.intValue(), 4); - // EngineDependentOperation op = new RowRemovalOperation(engine_config); EngineDependentOperation op = new ColumnAdditionByFetchingURLsOperation(engine_config, "fruits", "\"https://www.random.org/integers/?num=1&min=1&max=100&col=1&base=10&format=plain&rnd=new&city=\"+value", From 22124ac57ecc901e38929ebbe37ace16941226fe Mon Sep 17 00:00:00 2001 From: Antonin Delpeuch Date: Thu, 9 Mar 2017 00:21:34 +0000 Subject: [PATCH 5/5] Add checkbox to disable caching --- .../AddColumnByFetchingURLsCommand.java | 4 +- ...ColumnAdditionByFetchingURLsOperation.java | 115 ++++++++++-------- .../refine/tests/model/UrlFetchingTests.java | 3 +- .../core/langs/translation-default.json | 1 + .../modules/core/langs/translation-en.json | 1 + .../modules/core/langs/translation-fr.json | 1 + .../add-column-by-fetching-urls-dialog.html | 7 +- .../views/data-table/menu-edit-column.js | 4 +- 8 files changed, 80 insertions(+), 56 deletions(-) diff --git a/main/src/com/google/refine/commands/column/AddColumnByFetchingURLsCommand.java b/main/src/com/google/refine/commands/column/AddColumnByFetchingURLsCommand.java index ef6baf41c..717fb100f 100644 --- a/main/src/com/google/refine/commands/column/AddColumnByFetchingURLsCommand.java +++ b/main/src/com/google/refine/commands/column/AddColumnByFetchingURLsCommand.java @@ -54,6 +54,7 @@ public class AddColumnByFetchingURLsCommand extends EngineDependentCommand { int columnInsertIndex = Integer.parseInt(request.getParameter("columnInsertIndex")); int delay = Integer.parseInt(request.getParameter("delay")); String onError = request.getParameter("onError"); + boolean cacheResponses = Boolean.parseBoolean(request.getParameter("cacheResponses")); return new ColumnAdditionByFetchingURLsOperation( engineConfig, @@ -62,7 +63,8 @@ public class AddColumnByFetchingURLsCommand extends EngineDependentCommand { TextTransformOperation.stringToOnError(onError), newColumnName, columnInsertIndex, - delay + delay, + cacheResponses ); } diff --git a/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java b/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java index 445383101..f64f98c58 100644 --- a/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java +++ b/main/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperation.java @@ -85,6 +85,7 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat final protected String _newColumnName; final protected int _columnInsertIndex; final protected int _delay; + final protected boolean _cacheResponses; static public AbstractOperation reconstruct(Project project, JSONObject obj) throws Exception { JSONObject engineConfig = obj.getJSONObject("engineConfig"); @@ -96,7 +97,8 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat TextTransformOperation.stringToOnError(obj.getString("onError")), obj.getString("newColumnName"), obj.getInt("columnInsertIndex"), - obj.getInt("delay") + obj.getInt("delay"), + obj.optBoolean("cacheResponses", false) // false for retro-compatibility ); } @@ -107,7 +109,8 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat OnError onError, String newColumnName, int columnInsertIndex, - int delay + int delay, + boolean cacheResponses ) { super(engineConfig); @@ -119,6 +122,7 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat _columnInsertIndex = columnInsertIndex; _delay = delay; + _cacheResponses = cacheResponses; } @Override @@ -135,6 +139,7 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat writer.key("urlExpression"); writer.value(_urlExpression); writer.key("onError"); writer.value(TextTransformOperation.onErrorToString(_onError)); writer.key("delay"); writer.value(_delay); + writer.key("cacheResponses"); writer.value(_cacheResponses); writer.endObject(); } @@ -165,7 +170,8 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat project, engine, eval, - getBriefDescription(null) + getBriefDescription(null), + _cacheResponses ); } @@ -181,35 +187,39 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat Project project, Engine engine, Evaluable eval, - String description + String description, + boolean cacheResponses ) throws JSONException { super(description); _project = project; _engine = engine; _eval = eval; _historyEntryID = HistoryEntry.allocateID(); - _urlCache = CacheBuilder.newBuilder() - .maximumSize(2048) - .expireAfterWrite(10, TimeUnit.MINUTES) - .build( - new CacheLoader() { - public Serializable load(String urlString) { - Serializable result = fetch(urlString); - try { - // Always sleep for the delay, no matter how long the - // request took. This is more responsible than substracting - // the time spend requesting the URL, because it naturally - // slows us down if the server is busy and takes a long time - // to reply. - if (_delay > 0) { - Thread.sleep(_delay); - } - } catch (InterruptedException e) { - return null; - } - return result; - } - }); + _urlCache = null; + if (cacheResponses) { + _urlCache = CacheBuilder.newBuilder() + .maximumSize(2048) + .expireAfterWrite(10, TimeUnit.MINUTES) + .build( + new CacheLoader() { + public Serializable load(String urlString) { + Serializable result = fetch(urlString); + try { + // Always sleep for the delay, no matter how long the + // request took. This is more responsible than substracting + // the time spend requesting the URL, because it naturally + // slows us down if the server is busy and takes a long time + // to reply. + if (_delay > 0) { + Thread.sleep(_delay); + } + } catch (InterruptedException e) { + return null; + } + return result; + } + }); + } } @Override @@ -250,8 +260,20 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat List responseBodies = new ArrayList(urls.size()); for (int i = 0; i < urls.size(); i++) { CellAtRow urlData = urls.get(i); - CellAtRow cellAtRow = cachedFetch(urlData); - if (cellAtRow != null) { + String urlString = urlData.cell.value.toString(); + + Serializable response = null; + if (_urlCache != null) { + response = cachedFetch(urlString); + } else { + response = fetch(urlString); + } + + if (response != null) { + CellAtRow cellAtRow = new CellAtRow( + urlData.row, + new Cell(response, null)); + responseBodies.add(cellAtRow); } @@ -259,7 +281,7 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat if (_canceled) { break; - } + } } if (!_canceled) { @@ -279,30 +301,21 @@ public class ColumnAdditionByFetchingURLsOperation extends EngineDependentOperat } } - CellAtRow cachedFetch(CellAtRow urlData) { - String urlString = urlData.cell.value.toString(); - - Serializable cellResult = null; - try { - cellResult = _urlCache.get(urlString); - } catch(ExecutionException e) { - } - - if (cellResult != null) { - return new CellAtRow( - urlData.row, - new Cell(cellResult, null)); - } - return null; - } + Serializable cachedFetch(String urlString) { + try { + return _urlCache.get(urlString); + } catch(ExecutionException e) { + return null; + } + } Serializable fetch(String urlString) { - URL url = null; - try { - url = new URL(urlString); - } catch (MalformedURLException e) { - return null; - } + URL url = null; + try { + url = new URL(urlString); + } catch (MalformedURLException e) { + return null; + } try { URLConnection urlConnection = url.openConnection(); diff --git a/main/tests/server/src/com/google/refine/tests/model/UrlFetchingTests.java b/main/tests/server/src/com/google/refine/tests/model/UrlFetchingTests.java index 02f2b7e8b..10419225f 100644 --- a/main/tests/server/src/com/google/refine/tests/model/UrlFetchingTests.java +++ b/main/tests/server/src/com/google/refine/tests/model/UrlFetchingTests.java @@ -134,7 +134,8 @@ public class UrlFetchingTests extends RefineTest { OnError.SetToBlank, "rand", 1, - 500); + 500, + true); ProcessManager pm = project.getProcessManager(); Process process = op.createProcess(project, options); process.startPerforming(pm); diff --git a/main/webapp/modules/core/langs/translation-default.json b/main/webapp/modules/core/langs/translation-default.json index ed2f48c33..6859d55f3 100644 --- a/main/webapp/modules/core/langs/translation-default.json +++ b/main/webapp/modules/core/langs/translation-default.json @@ -510,6 +510,7 @@ "on-error": "On error", "set-blank": "set to blank", "store-err": "store error", + "cache-responses": "Cache responses", "copy-val": "copy value from original column", "warning-col-name": "You must enter a column name.", "add-col-fetch": "Add column by fetching URLs based on column", diff --git a/main/webapp/modules/core/langs/translation-en.json b/main/webapp/modules/core/langs/translation-en.json index b8a3fef78..c11e03f4c 100644 --- a/main/webapp/modules/core/langs/translation-en.json +++ b/main/webapp/modules/core/langs/translation-en.json @@ -510,6 +510,7 @@ "on-error": "On error", "set-blank": "set to blank", "store-err": "store error", + "cache-responses": "Cache responses", "copy-val": "copy value from original column", "warning-col-name": "You must enter a column name.", "add-col-fetch": "Add column by fetching URLs based on column", diff --git a/main/webapp/modules/core/langs/translation-fr.json b/main/webapp/modules/core/langs/translation-fr.json index dc7079c97..e34bb7c22 100644 --- a/main/webapp/modules/core/langs/translation-fr.json +++ b/main/webapp/modules/core/langs/translation-fr.json @@ -510,6 +510,7 @@ "on-error": "En cas d’erreur", "set-blank": "vider la cellule", "store-err": "conserver l’erreur", + "cache-responses": "Mettre les réponses en cache", "copy-val": "copier la valeur depuis la colonne originale", "warning-col-name": "Vous devez indiquer un nom de colonne.", "add-col-fetch": "Ajouter une colonne en moissonnant les données depuis les URL d’une colonne", diff --git a/main/webapp/modules/core/scripts/views/data-table/add-column-by-fetching-urls-dialog.html b/main/webapp/modules/core/scripts/views/data-table/add-column-by-fetching-urls-dialog.html index 3c5ea822c..e51bc2e52 100644 --- a/main/webapp/modules/core/scripts/views/data-table/add-column-by-fetching-urls-dialog.html +++ b/main/webapp/modules/core/scripts/views/data-table/add-column-by-fetching-urls-dialog.html @@ -12,11 +12,14 @@ - + + + +

$EXPRESSION_PREVIEW_WIDGET$ @@ -27,4 +30,4 @@ - \ No newline at end of file + diff --git a/main/webapp/modules/core/scripts/views/data-table/menu-edit-column.js b/main/webapp/modules/core/scripts/views/data-table/menu-edit-column.js index 2943b2b36..6e06addc3 100644 --- a/main/webapp/modules/core/scripts/views/data-table/menu-edit-column.js +++ b/main/webapp/modules/core/scripts/views/data-table/menu-edit-column.js @@ -103,6 +103,7 @@ DataTableColumnHeaderUI.extendMenu(function(column, columnHeaderUI, menu) { elmts.or_views_onErr.text($.i18n._('core-views')["on-error"]); elmts.or_views_setBlank.text($.i18n._('core-views')["set-blank"]); elmts.or_views_storeErr.text($.i18n._('core-views')["store-err"]); + elmts.or_views_cacheResponses.text($.i18n._('core-views')["cache-responses"]); elmts.or_views_urlFetch.text($.i18n._('core-views')["url-fetch"]); elmts.okButton.html($.i18n._('core-buttons')["ok"]); elmts.cancelButton.text($.i18n._('core-buttons')["cancel"]); @@ -135,7 +136,8 @@ DataTableColumnHeaderUI.extendMenu(function(column, columnHeaderUI, menu) { newColumnName: columnName, columnInsertIndex: columnIndex + 1, delay: elmts.throttleDelayInput[0].value, - onError: $('input[name="dialog-onerror-choice"]:checked')[0].value + onError: $('input[name="dialog-onerror-choice"]:checked')[0].value, + cacheResponses: $('input[name="dialog-cache-responses"]')[0].checked, }, null, { modelsChanged: true }