diff options
23 files changed, 268 insertions, 315 deletions
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java index b437e961104..eb268546580 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java @@ -90,6 +90,7 @@ import java.util.stream.Collectors; import static com.yahoo.config.model.api.container.ContainerServiceType.CLUSTERCONTROLLER_CONTAINER; import static com.yahoo.config.model.api.container.ContainerServiceType.CONTAINER; import static com.yahoo.config.model.api.container.ContainerServiceType.LOGSERVER_CONTAINER; +import static com.yahoo.vespa.curator.Curator.CompletionWaiter; import static com.yahoo.vespa.config.server.filedistribution.FileDistributionUtil.getFileReferencesOnDisk; import static com.yahoo.vespa.config.server.tenant.TenantRepository.HOSTED_VESPA_TENANT; import static com.yahoo.yolean.Exceptions.uncheck; @@ -344,6 +345,54 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye return Deployment.prepared(session, this, hostProvisioner, tenant, timeout, clock, false); } + public Transaction deactivateCurrentActivateNew(Session active, LocalSession prepared, boolean ignoreStaleSessionFailure) { + SessionRepository sessionRepository = tenantRepository.getTenant(prepared.getTenantName()).getSessionRepository(); + Transaction transaction = sessionRepository.createActivateTransaction(prepared); + if (active != null) { + checkIfActiveHasChanged(prepared, active, ignoreStaleSessionFailure); + checkIfActiveIsNewerThanSessionToBeActivated(prepared.getSessionId(), active.getSessionId()); + transaction.add(active.createDeactivateTransaction().operations()); + } + return transaction; + } + + static void checkIfActiveHasChanged(LocalSession session, Session currentActiveSession, boolean ignoreStaleSessionFailure) { + long activeSessionAtCreate = session.getActiveSessionAtCreate(); + log.log(Level.FINE, currentActiveSession.logPre() + "active session id at create time=" + activeSessionAtCreate); + if (activeSessionAtCreate == 0) return; // No active session at create + + long sessionId = session.getSessionId(); + long currentActiveSessionSessionId = currentActiveSession.getSessionId(); + log.log(Level.FINE, currentActiveSession.logPre() + "sessionId=" + sessionId + + ", current active session=" + currentActiveSessionSessionId); + if (currentActiveSession.isNewerThan(activeSessionAtCreate) && + currentActiveSessionSessionId != sessionId) { + String errMsg = currentActiveSession.logPre() + "Cannot activate session " + + sessionId + " because the currently active session (" + + currentActiveSessionSessionId + ") has changed since session " + sessionId + + " was created (was " + activeSessionAtCreate + " at creation time)"; + if (ignoreStaleSessionFailure) { + log.warning(errMsg + " (Continuing because of force.)"); + } else { + throw new ActivationConflictException(errMsg); + } + } + } + + private static boolean isValidSession(Session session) { + return session != null; + } + + // As of now, config generation is based on session id, and config generation must be a monotonically + // increasing number + static void checkIfActiveIsNewerThanSessionToBeActivated(long sessionId, long currentActiveSessionId) { + if (sessionId < currentActiveSessionId) { + throw new ActivationConflictException("It is not possible to activate session " + sessionId + + ", because it is older than current active session (" + + currentActiveSessionId + ")"); + } + } + // ---------------- Application operations ---------------------------------------------------------------- /** @@ -605,6 +654,17 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye // ---------------- Session operations ---------------------------------------------------------------- + + + public CompletionWaiter activate(LocalSession session, Session previousActiveSession, ApplicationId applicationId, boolean ignoreSessionStaleFailure) { + CompletionWaiter waiter = session.getSessionZooKeeperClient().createActiveWaiter(); + NestedTransaction transaction = new NestedTransaction(); + transaction.add(deactivateCurrentActivateNew(previousActiveSession, session, ignoreSessionStaleFailure)); + hostProvisioner.ifPresent(provisioner -> provisioner.activate(transaction, applicationId, session.getAllocatedHosts().getHosts())); + transaction.commit(); + return waiter; + } + /** * Gets the active Session for the given application id. * diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java index 01a95e8ecdd..8c2e6027691 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java @@ -9,9 +9,6 @@ import com.yahoo.config.provision.DockerImage; import com.yahoo.config.provision.HostFilter; import com.yahoo.config.provision.Provisioner; import java.util.logging.Level; -import com.yahoo.transaction.NestedTransaction; -import com.yahoo.transaction.Transaction; -import com.yahoo.vespa.config.server.ActivationConflictException; import com.yahoo.vespa.config.server.ApplicationRepository; import com.yahoo.vespa.config.server.ApplicationRepository.ActionTimer; import com.yahoo.vespa.config.server.TimeoutBudget; @@ -145,11 +142,7 @@ public class Deployment implements com.yahoo.config.provision.Deployment { CompletionWaiter waiter; try (Lock lock = tenant.getApplicationRepo().lock(applicationId)) { previousActiveSession = applicationRepository.getActiveSession(applicationId); - waiter = session.createActiveWaiter(); - NestedTransaction transaction = new NestedTransaction(); - transaction.add(deactivateCurrentActivateNew(previousActiveSession, session, ignoreSessionStaleFailure)); - hostProvisioner.ifPresent(provisioner -> provisioner.activate(transaction, applicationId, session.getAllocatedHosts().getHosts())); - transaction.commit(); + waiter = applicationRepository.activate(session, previousActiveSession, applicationId, ignoreSessionStaleFailure); } catch (RuntimeException e) { throw e; @@ -189,51 +182,4 @@ public class Deployment implements com.yahoo.config.provision.Deployment { } } - private static Transaction deactivateCurrentActivateNew(Session active, LocalSession prepared, boolean ignoreStaleSessionFailure) { - Transaction transaction = prepared.createActivateTransaction(); - if (isValidSession(active)) { - checkIfActiveHasChanged(prepared, active, ignoreStaleSessionFailure); - checkIfActiveIsNewerThanSessionToBeActivated(prepared.getSessionId(), active.getSessionId()); - transaction.add(active.createDeactivateTransaction().operations()); - } - return transaction; - } - - private static boolean isValidSession(Session session) { - return session != null; - } - - private static void checkIfActiveHasChanged(LocalSession session, Session currentActiveSession, boolean ignoreStaleSessionFailure) { - long activeSessionAtCreate = session.getActiveSessionAtCreate(); - log.log(Level.FINE, currentActiveSession.logPre() + "active session id at create time=" + activeSessionAtCreate); - if (activeSessionAtCreate == 0) return; // No active session at create - - long sessionId = session.getSessionId(); - long currentActiveSessionSessionId = currentActiveSession.getSessionId(); - log.log(Level.FINE, currentActiveSession.logPre() + "sessionId=" + sessionId + - ", current active session=" + currentActiveSessionSessionId); - if (currentActiveSession.isNewerThan(activeSessionAtCreate) && - currentActiveSessionSessionId != sessionId) { - String errMsg = currentActiveSession.logPre() + "Cannot activate session " + - sessionId + " because the currently active session (" + - currentActiveSessionSessionId + ") has changed since session " + sessionId + - " was created (was " + activeSessionAtCreate + " at creation time)"; - if (ignoreStaleSessionFailure) { - log.warning(errMsg + " (Continuing because of force.)"); - } else { - throw new ActivationConflictException(errMsg); - } - } - } - - // As of now, config generation is based on session id, and config generation must be a monotonically - // increasing number - private static void checkIfActiveIsNewerThanSessionToBeActivated(long sessionId, long currentActiveSessionId) { - if (sessionId < currentActiveSessionId) { - throw new ActivationConflictException("It is not possible to activate session " + sessionId + - ", because it is older than current active session (" + - currentActiveSessionId + ")"); - } - } - } diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSession.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSession.java index 96324ea4320..f842578b657 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSession.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSession.java @@ -1,15 +1,8 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.config.server.session; -import com.yahoo.config.application.api.ApplicationFile; -import com.yahoo.config.application.api.ApplicationMetaData; import com.yahoo.config.application.api.ApplicationPackage; import com.yahoo.config.provision.TenantName; -import com.yahoo.path.Path; -import com.yahoo.transaction.Transaction; -import com.yahoo.vespa.config.server.application.TenantApplications; - -import static com.yahoo.vespa.curator.Curator.CompletionWaiter; /** * A LocalSession is a session that has been created locally on this configserver. A local session can be edited and @@ -22,65 +15,14 @@ import static com.yahoo.vespa.curator.Curator.CompletionWaiter; // TODO: Separate the "application store" and "session" aspects - the latter belongs in the HTTP layer -bratseth public class LocalSession extends Session { - protected final ApplicationPackage applicationPackage; - private final TenantApplications applicationRepo; - /** * Creates a session. This involves loading the application, validating it and distributing it. * * @param sessionId The session id for this session. */ public LocalSession(TenantName tenant, long sessionId, ApplicationPackage applicationPackage, - SessionZooKeeperClient sessionZooKeeperClient, - TenantApplications applicationRepo) { - super(tenant, sessionId, sessionZooKeeperClient); - this.applicationPackage = applicationPackage; - this.applicationRepo = applicationRepo; - } - - public ApplicationFile getApplicationFile(Path relativePath, Mode mode) { - if (mode.equals(Mode.WRITE)) { - markSessionEdited(); - } - return applicationPackage.getFile(relativePath); - } - - void setPrepared() { - setStatus(Session.Status.PREPARE); - } - - private Transaction createSetStatusTransaction(Status status) { - return sessionZooKeeperClient.createWriteStatusTransaction(status); - } - - private void setStatus(Session.Status newStatus) { - sessionZooKeeperClient.writeStatus(newStatus); - } - - public CompletionWaiter createActiveWaiter() { - return sessionZooKeeperClient.createActiveWaiter(); - } - - public Transaction createActivateTransaction() { - Transaction transaction = createSetStatusTransaction(Status.ACTIVATE); - transaction.add(applicationRepo.createPutTransaction(sessionZooKeeperClient.readApplicationId(), getSessionId()).operations()); - return transaction; + SessionZooKeeperClient sessionZooKeeperClient) { + super(tenant, sessionId, sessionZooKeeperClient, applicationPackage); } - private void markSessionEdited() { - setStatus(Session.Status.NEW); - } - - public long getActiveSessionAtCreate() { - return applicationPackage.getMetaData().getPreviousActiveGeneration(); - } - - public enum Mode { - READ, WRITE - } - - public ApplicationMetaData getMetaData() { return applicationPackage.getMetaData(); } - - public ApplicationPackage getApplicationPackage() { return applicationPackage; } - } diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSession.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSession.java index 9677c7cf20e..642ac33ab09 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSession.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSession.java @@ -1,7 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.config.server.session; -import com.yahoo.config.application.api.ApplicationMetaData; import com.yahoo.config.application.api.ApplicationPackage; import com.yahoo.config.provision.AllocatedHosts; import com.yahoo.config.provision.TenantName; @@ -102,7 +101,7 @@ public class RemoteSession extends Session { KeeperException.NodeExistsException.class); Class<? extends Throwable> exceptionClass = e.getCause().getClass(); if (acceptedExceptions.contains(exceptionClass)) - log.log(Level.INFO, "Not able to notify completion for session " + getSessionId() + + log.log(Level.FINE, "Not able to notify completion for session " + getSessionId() + " (" + completionWaiter + ")," + " node " + (exceptionClass.equals(KeeperException.NoNodeException.class) ? "has been deleted" @@ -118,8 +117,4 @@ public class RemoteSession extends Session { transaction.close(); } - public ApplicationMetaData getMetaData() { - return sessionZooKeeperClient.loadApplicationPackage().getMetaData(); - } - } diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/Session.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/Session.java index a6818d1e43f..0fc85b5e51a 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/Session.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/Session.java @@ -3,11 +3,15 @@ package com.yahoo.vespa.config.server.session; import com.yahoo.component.Version; import com.yahoo.config.FileReference; +import com.yahoo.config.application.api.ApplicationFile; +import com.yahoo.config.application.api.ApplicationMetaData; +import com.yahoo.config.application.api.ApplicationPackage; import com.yahoo.config.provision.AllocatedHosts; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.AthenzDomain; import com.yahoo.config.provision.DockerImage; import com.yahoo.config.provision.TenantName; +import com.yahoo.path.Path; import com.yahoo.transaction.Transaction; import com.yahoo.vespa.config.server.tenant.TenantRepository; @@ -27,13 +31,24 @@ public abstract class Session implements Comparable<Session> { private final long sessionId; protected final TenantName tenant; protected final SessionZooKeeperClient sessionZooKeeperClient; + protected final Optional<ApplicationPackage> applicationPackage; protected Session(TenantName tenant, long sessionId, SessionZooKeeperClient sessionZooKeeperClient) { this.tenant = tenant; this.sessionId = sessionId; this.sessionZooKeeperClient = sessionZooKeeperClient; + this.applicationPackage = Optional.empty(); } + protected Session(TenantName tenant, long sessionId, SessionZooKeeperClient sessionZooKeeperClient, + ApplicationPackage applicationPackage) { + this.tenant = tenant; + this.sessionId = sessionId; + this.sessionZooKeeperClient = sessionZooKeeperClient; + this.applicationPackage = Optional.of(applicationPackage); + } + + public final long getSessionId() { return sessionId; } @@ -42,11 +57,19 @@ public abstract class Session implements Comparable<Session> { return sessionZooKeeperClient.readStatus(); } + public SessionZooKeeperClient getSessionZooKeeperClient() { + return sessionZooKeeperClient; + } + @Override public String toString() { return "Session,id=" + sessionId; } + public long getActiveSessionAtCreate() { + return getMetaData().getPreviousActiveGeneration(); + } + /** * The status of this session. */ @@ -135,6 +158,31 @@ public abstract class Session implements Comparable<Session> { // Note: Assumes monotonically increasing session ids public boolean isNewerThan(long sessionId) { return getSessionId() > sessionId; } + public ApplicationMetaData getMetaData() { + return applicationPackage.isPresent() + ? applicationPackage.get().getMetaData() + : sessionZooKeeperClient.loadApplicationPackage().getMetaData(); + } + + public ApplicationPackage getApplicationPackage() { + return applicationPackage.orElseThrow(() -> new RuntimeException("No application package found for " + this)); + } + + public ApplicationFile getApplicationFile(Path relativePath, LocalSession.Mode mode) { + if (mode.equals(Session.Mode.WRITE)) { + markSessionEdited(); + } + return getApplicationPackage().getFile(relativePath); + } + + private void markSessionEdited() { + setStatus(Session.Status.NEW); + } + + void setStatus(Session.Status newStatus) { + sessionZooKeeperClient.writeStatus(newStatus); + } + @Override public int compareTo(Session rhs) { Long lhsId = getSessionId(); @@ -142,4 +190,8 @@ public abstract class Session implements Comparable<Session> { return lhsId.compareTo(rhsId); } + public enum Mode { + READ, WRITE + } + } diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java index dccb4aa42c3..b38d90d470f 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java @@ -165,7 +165,7 @@ public class SessionRepository { getSessionAppDir(sessionId), session.getApplicationPackage(), sessionZooKeeperClient) .getConfigChangeActions(); - session.setPrepared(); + setPrepared(session); waiter.awaitCompletion(params.getTimeoutBudget().timeLeft()); return actions; } @@ -488,7 +488,7 @@ public class SessionRepository { SessionZooKeeperClient sessionZKClient = createSessionZooKeeperClient(sessionId); sessionZKClient.createNewSession(clock.instant()); Curator.CompletionWaiter waiter = sessionZKClient.getUploadWaiter(); - LocalSession session = new LocalSession(tenantName, sessionId, applicationPackage, sessionZKClient, applicationRepo); + LocalSession session = new LocalSession(tenantName, sessionId, applicationPackage, sessionZKClient); waiter.awaitCompletion(timeoutBudget.timeLeft()); return session; } @@ -546,7 +546,7 @@ public class SessionRepository { ApplicationPackage applicationPackage = createApplicationPackage(applicationFile, applicationId, sessionId, currentlyActiveSessionId, false); SessionZooKeeperClient sessionZooKeeperClient = createSessionZooKeeperClient(sessionId); - return new LocalSession(tenantName, sessionId, applicationPackage, sessionZooKeeperClient, applicationRepo); + return new LocalSession(tenantName, sessionId, applicationPackage, sessionZooKeeperClient); } catch (Exception e) { throw new RuntimeException("Error creating session " + sessionId, e); } @@ -595,7 +595,7 @@ public class SessionRepository { File sessionDir = getAndValidateExistingSessionAppDir(sessionId); ApplicationPackage applicationPackage = FilesApplicationPackage.fromFile(sessionDir); SessionZooKeeperClient sessionZKClient = createSessionZooKeeperClient(sessionId); - return new LocalSession(tenantName, sessionId, applicationPackage, sessionZKClient, applicationRepo); + return new LocalSession(tenantName, sessionId, applicationPackage, sessionZKClient); } /** @@ -696,6 +696,20 @@ public class SessionRepository { return locksPath.append(String.valueOf(sessionId)); } + public Transaction createActivateTransaction(Session session) { + Transaction transaction = createSetStatusTransaction(session, Session.Status.ACTIVATE); + transaction.add(applicationRepo.createPutTransaction(session.sessionZooKeeperClient.readApplicationId(), session.getSessionId()).operations()); + return transaction; + } + + private Transaction createSetStatusTransaction(Session session, Session.Status status) { + return session.sessionZooKeeperClient.createWriteStatusTransaction(status); + } + + void setPrepared(Session session) { + session.setStatus(Session.Status.PREPARE); + } + private static class FileTransaction extends AbstractTransaction { public static FileTransaction from(FileOperation operation) { diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java index 1b9527f4376..27440b3765b 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java @@ -53,7 +53,8 @@ public class SessionZooKeeperClient { private final String serverId; // hostname private final Optional<NodeFlavors> nodeFlavors; - // Only for testing when cache loader does not need cache entries. + // Only for testing + // TODO: Remove, use the constructor below public SessionZooKeeperClient(Curator curator, Path sessionPath) { this(curator, ConfigCurator.create(curator), sessionPath, "1", Optional.empty()); } @@ -93,7 +94,7 @@ public class SessionZooKeeperClient { return createCompletionWaiter(PREPARE_BARRIER); } - Curator.CompletionWaiter createActiveWaiter() { + public Curator.CompletionWaiter createActiveWaiter() { return createCompletionWaiter(ACTIVE_BARRIER); } diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java index 50a8cac2837..e42fb78f595 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java @@ -325,6 +325,10 @@ public class ApplicationRepositoryTest { { PrepareResult prepareResult = deployApp(testApp); + + assertNotNull(applicationRepository.getActiveSession(applicationId())); + assertNotNull(sessionRepository.getLocalSession(prepareResult.sessionId())); + try { applicationRepository.delete(applicationId(), Duration.ZERO); fail("Should have gotten an exception"); @@ -334,8 +338,7 @@ public class ApplicationRepositoryTest { // No active session or remote session (deleted in step above), but an exception was thrown above // A new delete should cleanup and be successful - RemoteSession activeSession = applicationRepository.getActiveSession(applicationId()); - assertNull(activeSession); + assertNull(applicationRepository.getActiveSession(applicationId())); assertNull(sessionRepository.getLocalSession(prepareResult.sessionId())); assertTrue(applicationRepository.delete(applicationId())); diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionCreateHandlerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionCreateHandlerTest.java index d2f09b802da..c6d241ee534 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionCreateHandlerTest.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionCreateHandlerTest.java @@ -12,7 +12,7 @@ import com.yahoo.vespa.config.server.application.CompressedApplicationInputStrea import com.yahoo.vespa.config.server.application.OrchestratorMock; import com.yahoo.vespa.config.server.http.HttpErrorResponse; import com.yahoo.vespa.config.server.http.SessionHandlerTest; -import com.yahoo.vespa.config.server.session.LocalSession; +import com.yahoo.vespa.config.server.session.Session; import com.yahoo.vespa.config.server.tenant.TenantRepository; import org.junit.Before; import org.junit.Ignore; @@ -135,7 +135,7 @@ public class SessionCreateHandlerTest extends SessionHandlerTest { public void require_that_handler_unpacks_application() throws IOException { File outFile = CompressedApplicationInputStreamTest.createTarFile(); createHandler().handle(post(outFile)); - ApplicationFile applicationFile = applicationRepository.getApplicationFileFromSession(tenant, 2, "services.xml", LocalSession.Mode.READ); + ApplicationFile applicationFile = applicationRepository.getApplicationFileFromSession(tenant, 2, "services.xml", Session.Mode.READ); assertTrue(applicationFile.exists()); } diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/session/LocalSessionTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/session/LocalSessionTest.java index a2ef6aeb578..cf01c9b6713 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/session/LocalSessionTest.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/session/LocalSessionTest.java @@ -77,31 +77,21 @@ public class LocalSessionTest { } @Test - public void require_that_session_status_is_updated() throws Exception { - LocalSession session = createSession(TenantName.defaultName(), 3); - assertThat(session.getStatus(), is(Session.Status.NEW)); - doPrepare(session); - assertThat(session.getStatus(), is(Session.Status.PREPARE)); - session.createActivateTransaction().commit(); - assertThat(session.getStatus(), is(Session.Status.ACTIVATE)); - } - - @Test public void require_that_marking_session_modified_changes_status_to_new() throws Exception { LocalSession session = createSession(TenantName.defaultName(), 3); doPrepare(session); assertThat(session.getStatus(), is(Session.Status.PREPARE)); - session.getApplicationFile(Path.createRoot(), LocalSession.Mode.READ); + session.getApplicationFile(Path.createRoot(), Session.Mode.READ); assertThat(session.getStatus(), is(Session.Status.PREPARE)); - session.getApplicationFile(Path.createRoot(), LocalSession.Mode.WRITE); + session.getApplicationFile(Path.createRoot(), Session.Mode.WRITE); assertThat(session.getStatus(), is(Session.Status.NEW)); } @Test public void require_that_application_file_can_be_fetched() throws Exception { LocalSession session = createSession(TenantName.defaultName(), 3); - ApplicationFile f1 = session.getApplicationFile(Path.fromString("services.xml"), LocalSession.Mode.READ); - ApplicationFile f2 = session.getApplicationFile(Path.fromString("services2.xml"), LocalSession.Mode.READ); + ApplicationFile f1 = session.getApplicationFile(Path.fromString("services.xml"), Session.Mode.READ); + ApplicationFile f2 = session.getApplicationFile(Path.fromString("services2.xml"), Session.Mode.READ); assertTrue(f1.exists()); assertFalse(f2.exists()); } @@ -127,7 +117,7 @@ public class LocalSessionTest { zkClient.write(Collections.singletonMap(new Version(0, 0, 0), new MockFileRegistry())); TenantApplications applications = tenantRepository.getTenant(tenantName).getApplicationRepo(); applications.createApplication(applicationId()); - LocalSession session = new LocalSession(tenant, sessionId, FilesApplicationPackage.fromFile(testApp), zkc, applications); + LocalSession session = new LocalSession(tenant, sessionId, FilesApplicationPackage.fromFile(testApp), zkc); session.setApplicationId(applicationId()); return session; } diff --git a/container-search/src/main/java/com/yahoo/prelude/Location.java b/container-search/src/main/java/com/yahoo/prelude/Location.java index f6228509bbb..3e9c2382f31 100644 --- a/container-search/src/main/java/com/yahoo/prelude/Location.java +++ b/container-search/src/main/java/com/yahoo/prelude/Location.java @@ -24,16 +24,9 @@ public class Location { private int y2 = 1; // center(x,y), radius - private int x = 1; - private int y = 1; - private int r = 1; - - // next three are now UNUSED - // ranking table, rank multiplier (scale) - // {0, 1} an int to make parsing and rendering the hit even simpler - private int tableId = 0; - private int s = 1; - private int replace = 0; + private int x = 0; + private int y = 0; + private int r = -1; private boolean renderCircle = false; private boolean renderRectangle = false; @@ -47,14 +40,14 @@ public class Location { return dimensions == l.dimensions && renderCircle == l.renderCircle && renderRectangle == l.renderRectangle - && aspect == l.aspect - && x1 == l.x1 - && x2 == l.x2 - && y1 == l.y1 - && y2 == l.y2 - && x == l.x - && y == l.y - && r == l.r; + && this.aspect == l.aspect + && this.x1 == l.x1 + && this.x2 == l.x2 + && this.y1 == l.y1 + && this.y2 == l.y2 + && this.x == l.x + && this.y == l.y + && this.r == l.r; } public boolean hasDimensions() { @@ -64,10 +57,10 @@ public class Location { if (hasDimensions() && dimensions != d) { throw new IllegalArgumentException("already has dimensions="+dimensions+", cannot change it to "+d); } - if (d == 1 || d == 2) { + if (d == 2) { dimensions = d; } else { - throw new IllegalArgumentException("Illegal location, dimensions must be 1 or 2, but was: "+d); + throw new IllegalArgumentException("Illegal location, dimensions must be 2, but was: "+d); } } public int getDimensions() { @@ -89,13 +82,13 @@ public class Location { if (px1 > px2) { throw new IllegalArgumentException("cannot have w > e"); } - x1 = px1; - x2 = px2; + this.x1 = px1; + this.x2 = px2; if (py1 > py2) { throw new IllegalArgumentException("cannot have s > n"); } - y1 = py1; - y2 = py2; + this.y1 = py1; + this.y2 = py2; renderRectangle = true; } @@ -104,12 +97,12 @@ public class Location { //no need to "optimize" for special cases, exactly 0, 30, 45, 60, or 90 degrees won't be input anyway double degrees = (double) y / 1000000d; if (degrees <= -90.0 || degrees >= +90.0) { - aspect = 0; + this.aspect = 0; return; } double radians = degrees * Math.PI / 180d; double cosLatRadians = Math.cos(radians); - aspect = (long) (cosLatRadians * 4294967295L); + this.aspect = (long) (cosLatRadians * 4294967295L); } public void setGeoCircle(double ns, double ew, double radius_in_degrees) { @@ -129,9 +122,9 @@ public class Location { if (radius_in_degrees < 0) { pr = -1; } - x = px; - y = py; - r = pr; + this.x = px; + this.y = py; + this.r = pr; renderCircle = true; adjustAspect(); } @@ -144,9 +137,9 @@ public class Location { if (radius_in_units < 0) { radius_in_units = -1; } - x = px; - y = py; - r = radius_in_units; + this.x = px; + this.y = py; + this.r = radius_in_units; renderCircle = true; } @@ -158,17 +151,12 @@ public class Location { String rectPart = rectangle.substring(1,endof); StringTokenizer tokens = new StringTokenizer(rectPart, ","); setDimensions(Integer.parseInt(tokens.nextToken())); - if (dimensions == 1) { - x1 = Integer.parseInt(tokens.nextToken()); - x2 = Integer.parseInt(tokens.nextToken()); - if (tokens.hasMoreTokens()) { - throw new IllegalArgumentException("Illegal location syntax: "+rectangle); - } - } else if (dimensions == 2) { - x1 = Integer.parseInt(tokens.nextToken()); - y1 = Integer.parseInt(tokens.nextToken()); - x2 = Integer.parseInt(tokens.nextToken()); - y2 = Integer.parseInt(tokens.nextToken()); + this.x1 = Integer.parseInt(tokens.nextToken()); + this.y1 = Integer.parseInt(tokens.nextToken()); + this.x2 = Integer.parseInt(tokens.nextToken()); + this.y2 = Integer.parseInt(tokens.nextToken()); + if (tokens.hasMoreTokens()) { + throw new IllegalArgumentException("Illegal location syntax: "+rectangle); } renderRectangle = true; String theRest = rectangle.substring(endof+1).trim(); @@ -185,34 +173,24 @@ public class Location { String circlePart = circle.substring(1,endof); StringTokenizer tokens = new StringTokenizer(circlePart, ","); setDimensions(Integer.parseInt(tokens.nextToken())); - x = Integer.parseInt(tokens.nextToken()); - if (dimensions == 2) { - y = Integer.parseInt(tokens.nextToken()); - } - r = Integer.parseInt(tokens.nextToken()); + this.x = Integer.parseInt(tokens.nextToken()); + this.y = Integer.parseInt(tokens.nextToken()); + this.r = Integer.parseInt(tokens.nextToken()); Integer.parseInt(tokens.nextToken()); // was "tableId" - Integer.parseInt(tokens.nextToken()); // was "scale" (multiplier) + Integer.parseInt(tokens.nextToken()); // was "scale" Integer.parseInt(tokens.nextToken()); // was "replace" - - if (dimensions == 1) { - if (tokens.hasMoreTokens()) { - throw new IllegalArgumentException("Illegal location syntax: "+circle); - } - } - else { - if (tokens.hasMoreTokens()) { - String aspectToken = tokens.nextToken(); - if (aspectToken.equalsIgnoreCase("CalcLatLon")) { - adjustAspect(); - } else { - try { - aspect = Long.parseLong(aspectToken); - } catch (NumberFormatException nfe) { - throw new IllegalArgumentException("Aspect "+aspectToken+" for location must be an integer or 'CalcLatLon' for automatic aspect calculation.", nfe); - } - if (aspect > 4294967295L || aspect < 0) { - throw new IllegalArgumentException("Aspect "+aspect+" for location parameter must be less than 4294967296 (2^32)"); - } + if (tokens.hasMoreTokens()) { + String aspectToken = tokens.nextToken(); + if (aspectToken.equalsIgnoreCase("CalcLatLon")) { + adjustAspect(); + } else { + try { + aspect = Long.parseLong(aspectToken); + } catch (NumberFormatException nfe) { + throw new IllegalArgumentException("Aspect "+aspectToken+" for location must be an integer or 'CalcLatLon' for automatic aspect calculation.", nfe); + } + if (aspect > 4294967295L || aspect < 0) { + throw new IllegalArgumentException("Aspect "+aspect+" for location parameter must be less than 4294967296 (2^32)"); } } } @@ -261,28 +239,20 @@ public class Location { } if (renderRectangle) { ser.append("[").append(dimensions).append(","); - if (dimensions == 1) { - ser.append(x1).append(","). - append(x2); - } - else { - ser.append(x1).append(","). - append(y1).append(","). - append(x2).append(","). - append(y2); - } + ser.append(x1).append(","). + append(y1).append(","). + append(x2).append(","). + append(y2); ser.append("]"); } if (renderCircle) { - ser.append("(").append(dimensions).append(",").append(x); - if (dimensions == 2) { - ser.append(",").append(y); - } - ser.append(",").append(forBackend ? backendRadius() : r). - append(",").append(tableId). - append(",").append(s). - append(",").append(replace); - if (dimensions == 2 && aspect != 0) { + ser.append("(").append(dimensions).append(","). + append(this.x).append(",").append(this.y); + ser.append(",").append(forBackend ? backendRadius() : this.r). + append(",").append(0). // was "tableId" + append(",").append(1). // was "scale" + append(",").append(0); // was "replace" + if (aspect != 0) { ser.append(",").append(aspect); } ser.append(")"); @@ -296,7 +266,7 @@ public class Location { */ public int getBoundingWidth() { if (renderCircle) { - return r * 2; + return this.r * 2; } else { return x2 - x1; } @@ -308,7 +278,7 @@ public class Location { */ public int getBoundingHeight() { if (renderCircle) { - return r * 2; + return this.r * 2; } else { return y2 - y1; } @@ -370,11 +340,11 @@ public class Location { **/ public double degRadius() { checkGeoCircle(); - return (r < 0) ? -1.0 : (0.000001 * r); + return (this.r < 0) ? -1.0 : (0.000001 * this.r); } private int backendRadius() { - return (r < 0) ? -1 : r; + return (this.r < 0) ? -1 : this.r; } /** diff --git a/container-search/src/main/java/com/yahoo/search/yql/ProgramParser.java b/container-search/src/main/java/com/yahoo/search/yql/ProgramParser.java index a270d935a97..2c593e3f283 100644 --- a/container-search/src/main/java/com/yahoo/search/yql/ProgramParser.java +++ b/container-search/src/main/java/com/yahoo/search/yql/ProgramParser.java @@ -139,7 +139,7 @@ final class ProgramParser { int charPositionInLine, @NotNull String msg, @Nullable RecognitionException e) { - throw new ProgramCompileException(new Location(programName, line, charPositionInLine), msg); + throw new ProgramCompileException(new Location(programName, line, charPositionInLine), "%s", msg); } }); @@ -156,7 +156,7 @@ final class ProgramParser { int charPositionInLine, @NotNull String msg, @Nullable RecognitionException e) { - throw new ProgramCompileException(new Location(programName, line, charPositionInLine), msg); + throw new ProgramCompileException(new Location(programName, line, charPositionInLine), "%s", msg); } }); diff --git a/container-search/src/test/java/com/yahoo/vespa/streamingvisitors/VdsVisitorTestCase.java b/container-search/src/test/java/com/yahoo/vespa/streamingvisitors/VdsVisitorTestCase.java index 7841b6f715c..4bfbd91eb89 100644 --- a/container-search/src/test/java/com/yahoo/vespa/streamingvisitors/VdsVisitorTestCase.java +++ b/container-search/src/test/java/com/yahoo/vespa/streamingvisitors/VdsVisitorTestCase.java @@ -130,7 +130,7 @@ public class VdsVisitorTestCase { traceLevel = 100; summary = "fancysummary"; profile = "fancyprofile"; - location = "(1,10000,2000,0,1,0)"; + location = "(2,10000,2000,0,0,1,0)"; sortSpec = "+surname -yearofbirth"; rankProperties = "rankfeature.something=2"; diff --git a/searchcore/src/tests/proton/matching/query_test.cpp b/searchcore/src/tests/proton/matching/query_test.cpp index 01f7e25eb51..7cc11b0dc0c 100644 --- a/searchcore/src/tests/proton/matching/query_test.cpp +++ b/searchcore/src/tests/proton/matching/query_test.cpp @@ -698,7 +698,7 @@ void Test::requireThatQueryGluesEverythingTogether() { ASSERT_TRUE(search.get()); } -void checkQueryAddsLocation(Test &test, const string &loc_string) { +void checkQueryAddsLocation(Test &test, const string &loc_in, const string &loc_out) { fef_test::IndexEnvironment index_environment; FieldInfo field_info(FieldType::INDEX, CollectionType::SINGLE, field, 0); index_environment.getFields().push_back(field_info); @@ -712,7 +712,7 @@ void checkQueryAddsLocation(Test &test, const string &loc_string) { Query query; query.buildTree(stack_dump, - loc_field + ":" + loc_string, + loc_field + ":" + loc_in, ViewResolver(), index_environment); vector<const ITermData *> term_data; query.extractTerms(term_data); @@ -729,9 +729,9 @@ void checkQueryAddsLocation(Test &test, const string &loc_string) { query.fetchPostings(); SearchIterator::UP search = query.createSearch(*md); test.ASSERT_TRUE(search.get()); - if (!test.EXPECT_NOT_EQUAL(string::npos, search->asString().find(loc_string))) { - fprintf(stderr, "search (missing loc_string '%s'): %s", - loc_string.c_str(), search->asString().c_str()); + if (!test.EXPECT_NOT_EQUAL(string::npos, search->asString().find(loc_out))) { + fprintf(stderr, "search (missing loc_out '%s'): %s", + loc_out.c_str(), search->asString().c_str()); } } @@ -790,11 +790,15 @@ void Test::requireThatLocationIsAddedTheCorrectPlace() { } void Test::requireThatQueryAddsLocation() { - checkQueryAddsLocation(*this, "(2,10,10,3,0,1,0,0)"); + checkQueryAddsLocation(*this, "(2,10,10,3,0,1,0,0)", "{p:{x:10,y:10},r:3,b:{x:[7,13],y:[7,13]}}"); + checkQueryAddsLocation(*this, "{p:{x:10,y:10},r:3}", "{p:{x:10,y:10},r:3,b:{x:[7,13],y:[7,13]}}"); + checkQueryAddsLocation(*this, "{b:{x:[6,11],y:[8,15]},p:{x:10,y:10},r:3}", "{p:{x:10,y:10},r:3,b:{x:[7,11],y:[8,13]}}"); + checkQueryAddsLocation(*this, "{a:12345,b:{x:[8,10],y:[8,10]},p:{x:10,y:10},r:3}", "{p:{x:10,y:10},r:3,a:12345,b:{x:[8,10],y:[8,10]}}"); } void Test::requireThatQueryAddsLocationCutoff() { - checkQueryAddsLocation(*this, "[2,10,10,20,20]"); + checkQueryAddsLocation(*this, "[2,10,11,23,24]", "{b:{x:[10,23],y:[11,24]}}"); + checkQueryAddsLocation(*this, "{b:{y:[11,24],x:[10,23]}}", "{b:{x:[10,23],y:[11,24]}}"); } void diff --git a/searchcore/src/vespa/searchcore/proton/matching/query.cpp b/searchcore/src/vespa/searchcore/proton/matching/query.cpp index 65959e6e6de..994e26081e7 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/query.cpp +++ b/searchcore/src/vespa/searchcore/proton/matching/query.cpp @@ -83,7 +83,7 @@ GeoLocationSpec parse_location_string(string str) { return empty; } GeoLocationParser parser; - if (parser.parseOldFormatWithField(str)) { + if (parser.parseWithField(str)) { auto attr_name = PositionDataType::getZCurveFieldName(parser.getFieldName()); return GeoLocationSpec{attr_name, parser.getGeoLocation()}; } else { diff --git a/searchlib/src/tests/common/location/geo_location_test.cpp b/searchlib/src/tests/common/location/geo_location_test.cpp index eec2411f2af..d142733041e 100644 --- a/searchlib/src/tests/common/location/geo_location_test.cpp +++ b/searchlib/src/tests/common/location/geo_location_test.cpp @@ -18,12 +18,7 @@ using Aspect = search::common::GeoLocation::Aspect; constexpr int32_t plus_inf = std::numeric_limits<int32_t>::max(); constexpr int32_t minus_inf = std::numeric_limits<int32_t>::min(); -bool is_parseable(const char *str) { - GeoLocationParser parser; - return parser.parseOldFormat(str); -} - -bool is_parseable_new(const char *str, bool with_field = false) { +bool is_parseable(const char *str, bool with_field = false) { GeoLocationParser parser; if (with_field) { return parser.parseWithField(str); @@ -31,13 +26,7 @@ bool is_parseable_new(const char *str, bool with_field = false) { return parser.parseNoField(str); } -GeoLocation parse(const char *str) { - GeoLocationParser parser; - EXPECT_TRUE(parser.parseOldFormat(str)); - return parser.getGeoLocation(); -} - -GeoLocation parse_new(const std::string &str, bool with_field = false) { +GeoLocation parse(const std::string &str, bool with_field = false) { GeoLocationParser parser; if (with_field) { EXPECT_TRUE(parser.parseWithField(str)); @@ -59,12 +48,12 @@ TEST(GeoLocationParserTest, malformed_bounding_boxes_are_not_parseable) { } TEST(GeoLocationParserTest, new_bounding_box_formats) { - EXPECT_TRUE(is_parseable_new("{b:{x:[10,30],y:[20,40]}}")); - EXPECT_TRUE(is_parseable_new("{b:{}}")); - EXPECT_TRUE(is_parseable_new("{b:[]}")); - EXPECT_TRUE(is_parseable_new("{b:10,b:20}")); - EXPECT_TRUE(is_parseable_new("{b:[10, 20, 30, 40]}")); - EXPECT_FALSE(is_parseable_new("{b:{x:[10,30],y:[20,40]}")); + EXPECT_TRUE(is_parseable("{b:{x:[10,30],y:[20,40]}}")); + EXPECT_TRUE(is_parseable("{b:{}}")); + EXPECT_TRUE(is_parseable("{b:[]}")); + EXPECT_TRUE(is_parseable("{b:10,b:20}")); + EXPECT_TRUE(is_parseable("{b:[10, 20, 30, 40]}")); + EXPECT_FALSE(is_parseable("{b:{x:[10,30],y:[20,40]}")); } TEST(GeoLocationParserTest, malformed_circles_are_not_parseable) { @@ -80,23 +69,23 @@ TEST(GeoLocationParserTest, malformed_circles_are_not_parseable) { } TEST(GeoLocationParserTest, new_circle_formats) { - EXPECT_TRUE(is_parseable_new("{p:{x:10,y:20}}")); - EXPECT_TRUE(is_parseable_new("{p:{x:10,y:20},r:5}")); - EXPECT_TRUE(is_parseable_new("{p:{x:10, y:10}, r:5}")); - EXPECT_TRUE(is_parseable_new("{'p':{y:20,x:10},'r':5}")); - EXPECT_TRUE(is_parseable_new("{\n \"p\": { \"x\": 10, \"y\": 20},\n \"r\": 5\n}")); + EXPECT_TRUE(is_parseable("{p:{x:10,y:20}}")); + EXPECT_TRUE(is_parseable("{p:{x:10,y:20},r:5}")); + EXPECT_TRUE(is_parseable("{p:{x:10, y:10}, r:5}")); + EXPECT_TRUE(is_parseable("{'p':{y:20,x:10},'r':5}")); + EXPECT_TRUE(is_parseable("{\n \"p\": { \"x\": 10, \"y\": 20},\n \"r\": 5\n}")); // json demands colon: - EXPECT_FALSE(is_parseable_new("{p:{x:10,y:10},r=5}")); + EXPECT_FALSE(is_parseable("{p:{x:10,y:10},r=5}")); // missing y -> 0 default: - EXPECT_TRUE(is_parseable_new("{p:{x:10},r:5}")); + EXPECT_TRUE(is_parseable("{p:{x:10},r:5}")); // unused extra fields are ignored: - EXPECT_TRUE(is_parseable_new("{p:{x:10,y:10,z:10},r:5,c:1,d:17}")); + EXPECT_TRUE(is_parseable("{p:{x:10,y:10,z:10},r:5,c:1,d:17}")); } TEST(GeoLocationParserTest, bounding_boxes_can_be_parsed) { for (const auto & loc : { parse("[2,10,20,30,40]"), - parse_new("{b:{x:[10,30],y:[20,40]}}") + parse("{b:{x:[10,30],y:[20,40]}}") }) { EXPECT_EQ(false, loc.has_point); EXPECT_EQ(true, loc.bounding_box.active()); @@ -114,7 +103,7 @@ TEST(GeoLocationParserTest, bounding_boxes_can_be_parsed) { TEST(GeoLocationParserTest, circles_can_be_parsed) { for (const auto & loc : { parse("(2,10,20,5,0,0,0)"), - parse_new("{p:{x:10,y:20},r:5}") + parse("{p:{x:10,y:20},r:5}") }) { EXPECT_EQ(true, loc.has_point); EXPECT_EQ(true, loc.bounding_box.active()); @@ -132,7 +121,7 @@ TEST(GeoLocationParserTest, circles_can_be_parsed) { TEST(GeoLocationParserTest, circles_can_have_aspect_ratio) { for (const auto & loc : { parse("(2,10,20,5,0,0,0,2147483648)"), - parse_new("{p:{x:10,y:20},r:5,a:2147483648}") + parse("{p:{x:10,y:20},r:5,a:2147483648}") }) { EXPECT_EQ(true, loc.has_point); EXPECT_EQ(true, loc.bounding_box.active()); @@ -145,14 +134,14 @@ TEST(GeoLocationParserTest, circles_can_have_aspect_ratio) { EXPECT_EQ(21, loc.bounding_box.x.high); EXPECT_EQ(25, loc.bounding_box.y.high); } - auto loc2 = parse_new("{p:{x:10,y:10},a:3123456789}"); + auto loc2 = parse("{p:{x:10,y:10},a:3123456789}"); EXPECT_EQ(3123456789, loc2.x_aspect.multiplier); } TEST(GeoLocationParserTest, bounding_box_can_be_specified_after_circle) { for (const auto & loc : { parse("(2,10,20,5,0,0,0)[2,10,20,30,40]"), - parse_new("{p:{x:10,y:20},r:5,b:{x:[10,30],y:[20,40]}}") + parse("{p:{x:10,y:20},r:5,b:{x:[10,30],y:[20,40]}}") }) { EXPECT_EQ(true, loc.has_point); EXPECT_EQ(true, loc.bounding_box.active()); @@ -170,7 +159,7 @@ TEST(GeoLocationParserTest, bounding_box_can_be_specified_after_circle) { TEST(GeoLocationParserTest, circles_can_be_specified_after_bounding_box) { for (const auto & loc : { parse("[2,10,20,30,40](2,10,20,5,0,0,0)"), - parse_new("{b:{x:[10,30],y:[20,40]},p:{x:10,y:20},r:5}") + parse("{b:{x:[10,30],y:[20,40]},p:{x:10,y:20},r:5}") }) { EXPECT_EQ(true, loc.has_point); EXPECT_EQ(true, loc.bounding_box.active()); @@ -183,7 +172,7 @@ TEST(GeoLocationParserTest, circles_can_be_specified_after_bounding_box) { EXPECT_EQ(15, loc.bounding_box.x.high); EXPECT_EQ(25, loc.bounding_box.y.high); } - const auto &loc = parse_new("{a:12345,b:{x:[8,10],y:[8,10]},p:{x:10,y:10},r:3}"); + const auto &loc = parse("{a:12345,b:{x:[8,10],y:[8,10]},p:{x:10,y:10},r:3}"); EXPECT_EQ(true, loc.has_point); EXPECT_EQ(10, loc.point.x); EXPECT_EQ(10, loc.point.y); @@ -193,7 +182,7 @@ TEST(GeoLocationParserTest, circles_can_be_specified_after_bounding_box) { TEST(GeoLocationParserTest, santa_search_gives_non_wrapped_bounding_box) { for (const auto & loc : { parse("(2,122163600,89998536,290112,4,2000,0,109704)"), - parse_new("{p:{x:122163600,y:89998536},r:290112,a:109704}") + parse("{p:{x:122163600,y:89998536},r:290112,a:109704}") }) { EXPECT_GE(loc.bounding_box.x.high, loc.bounding_box.x.low); EXPECT_GE(loc.bounding_box.y.high, loc.bounding_box.y.low); @@ -203,7 +192,7 @@ TEST(GeoLocationParserTest, santa_search_gives_non_wrapped_bounding_box) { TEST(GeoLocationParserTest, near_boundary_search_gives_non_wrapped_bounding_box) { for (const auto & loc1 : { parse("(2,2000000000,2000000000,3000000000,0,1,0)"), - parse_new("{p:{x:2000000000,y:2000000000},r:3000000000}") + parse("{p:{x:2000000000,y:2000000000},r:3000000000}") }) { EXPECT_GE(loc1.bounding_box.x.high, loc1.bounding_box.x.low); EXPECT_GE(loc1.bounding_box.y.high, loc1.bounding_box.y.low); @@ -213,7 +202,7 @@ TEST(GeoLocationParserTest, near_boundary_search_gives_non_wrapped_bounding_box) for (const auto & loc2 : { parse("(2,-2000000000,-2000000000,3000000000,0,1,0)"), - parse_new("{p:{x:-2000000000,y:-2000000000},r:3000000000}") + parse("{p:{x:-2000000000,y:-2000000000},r:3000000000}") }) { EXPECT_GE(loc2.bounding_box.x.high, loc2.bounding_box.x.low); EXPECT_GE(loc2.bounding_box.y.high, loc2.bounding_box.y.low); @@ -479,7 +468,7 @@ TEST(GeoLocationParserTest, can_parse_what_query_tree_produces) { search::query::Location loc_1(point_1); std::string str_1 = loc_1.getJsonFormatString(); - auto result_1 = parse_new(str_1); + auto result_1 = parse(str_1); EXPECT_EQ(true, result_1.has_point); EXPECT_EQ(false, result_1.has_radius()); @@ -490,7 +479,7 @@ TEST(GeoLocationParserTest, can_parse_what_query_tree_produces) { search::query::Location loc_1b(point_1, distance, aspect_ratio); std::string str_1b = loc_1b.getJsonFormatString(); - auto result_1b = parse_new(str_1b); + auto result_1b = parse(str_1b); EXPECT_EQ(true, result_1b.has_point); EXPECT_EQ(true, result_1b.has_radius()); @@ -505,7 +494,7 @@ TEST(GeoLocationParserTest, can_parse_what_query_tree_produces) { search::query::Location loc_2(rectangle_1); std::string str_2 = loc_2.getJsonFormatString(); - auto result_2 = parse_new(str_2); + auto result_2 = parse(str_2); EXPECT_EQ(false, result_2.has_point); EXPECT_EQ(false, result_2.has_radius()); @@ -518,7 +507,7 @@ TEST(GeoLocationParserTest, can_parse_what_query_tree_produces) { search::query::Location loc_3(rectangle_1, point_1, distance, aspect_ratio); std::string str_3 = loc_3.getJsonFormatString(); - auto result_3 = parse_new(str_3); + auto result_3 = parse(str_3); EXPECT_EQ(true, result_3.has_point); EXPECT_EQ(true, result_3.has_radius()); diff --git a/searchlib/src/vespa/searchlib/common/geo_location_parser.cpp b/searchlib/src/vespa/searchlib/common/geo_location_parser.cpp index 53792e56562..7d793f9eb55 100644 --- a/searchlib/src/vespa/searchlib/common/geo_location_parser.cpp +++ b/searchlib/src/vespa/searchlib/common/geo_location_parser.cpp @@ -63,19 +63,6 @@ GeoLocationParser::correctDimensionalitySkip(const char * &p) { } bool -GeoLocationParser::parseOldFormatWithField(const std::string &str) -{ - auto sep = str.find(':'); - if (sep == std::string::npos) { - _parseError = "Location string lacks field specification"; - return false; - } - _field_name = str.substr(0, sep); - std::string only_loc = str.substr(sep + 1); - return parseOldFormat(only_loc); -} - -bool GeoLocationParser::parseOldFormat(const std::string &locStr) { bool foundBoundingBox = false; diff --git a/searchlib/src/vespa/searchlib/common/geo_location_parser.h b/searchlib/src/vespa/searchlib/common/geo_location_parser.h index 7d725f4a035..e1a68163fbb 100644 --- a/searchlib/src/vespa/searchlib/common/geo_location_parser.h +++ b/searchlib/src/vespa/searchlib/common/geo_location_parser.h @@ -20,9 +20,6 @@ public: bool parseNoField(const std::string &locStr); bool parseWithField(const std::string &locStr); - bool parseOldFormat(const std::string &locStr); - bool parseOldFormatWithField(const std::string &str); - std::string getFieldName() const { return _field_name; } GeoLocation getGeoLocation() const; @@ -46,6 +43,7 @@ private: const char *_parseError; bool correctDimensionalitySkip(const char * &p); bool parseJsonFormat(const std::string &locStr); + bool parseOldFormat(const std::string &locStr); }; } // namespace diff --git a/searchlib/src/vespa/searchlib/features/distancefeature.cpp b/searchlib/src/vespa/searchlib/features/distancefeature.cpp index 78a94c4abe0..d2431ccc06b 100644 --- a/searchlib/src/vespa/searchlib/features/distancefeature.cpp +++ b/searchlib/src/vespa/searchlib/features/distancefeature.cpp @@ -83,8 +83,8 @@ feature_t DistanceExecutor::calculateDistance(uint32_t docId) { _best_index = -1.0; - _best_x = 0.0; - _best_y = 0.0; + _best_x = -180.0 * 1.0e6; + _best_y = 90.0 * 1.0e6; if ((! _locations.empty()) && (_pos != nullptr)) { LOG(debug, "calculate 2D Z-distance from %zu locations", _locations.size()); return calculate2DZDistance(docId); @@ -135,8 +135,8 @@ DistanceExecutor::execute(uint32_t docId) { outputs().set_number(0, calculateDistance(docId)); outputs().set_number(1, _best_index); - outputs().set_number(2, _best_y * 0.000001); // latitude - outputs().set_number(3, _best_x * 0.000001); // longitude + outputs().set_number(2, _best_y * 1.0e-6); // latitude + outputs().set_number(3, _best_x * 1.0e-6); // longitude } const feature_t DistanceExecutor::DEFAULT_DISTANCE(6400000000.0); diff --git a/searchlib/src/vespa/searchlib/query/tree/location.cpp b/searchlib/src/vespa/searchlib/query/tree/location.cpp index 44f0b82d304..9b45ba18b97 100644 --- a/searchlib/src/vespa/searchlib/query/tree/location.cpp +++ b/searchlib/src/vespa/searchlib/query/tree/location.cpp @@ -100,7 +100,7 @@ Location::getJsonFormatString() const } vespalib::asciistream &operator<<(vespalib::asciistream &out, const Location &loc) { - return out << loc.getOldFormatString(); + return out << loc.getJsonFormatString(); } } // namespace diff --git a/searchlib/src/vespa/searchlib/query/tree/stackdumpquerycreator.h b/searchlib/src/vespa/searchlib/query/tree/stackdumpquerycreator.h index a7e00d41555..66702fcd85c 100644 --- a/searchlib/src/vespa/searchlib/query/tree/stackdumpquerycreator.h +++ b/searchlib/src/vespa/searchlib/query/tree/stackdumpquerycreator.h @@ -144,7 +144,9 @@ private: t = &builder.addSuffixTerm(term, view, id, weight); } else if (type == ParseItem::ITEM_GEO_LOCATION_TERM) { search::common::GeoLocationParser parser; - parser.parseOldFormat(term); + if (! parser.parseNoField(term)) { + LOG(warning, "invalid geo location term '%s'", term.data()); + } Location loc(parser.getGeoLocation()); t = &builder.addLocationTerm(loc, view, id, weight); } else if (type == ParseItem::ITEM_NUMTERM) { diff --git a/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.cpp b/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.cpp index 1f9b553b56a..822017e0bdf 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.cpp @@ -76,7 +76,7 @@ GetDocsumsState::parse_locations() assert(_parsedLocations.empty()); // only allowed to call this once if (! _args.getLocation().empty()) { GeoLocationParser parser; - if (parser.parseOldFormatWithField(_args.getLocation())) { + if (parser.parseWithField(_args.getLocation())) { auto view = parser.getFieldName(); auto attr_name = PositionDataType::getZCurveFieldName(view); GeoLocationSpec spec{attr_name, parser.getGeoLocation()}; @@ -94,7 +94,7 @@ GetDocsumsState::parse_locations() vespalib::string view = iterator.getIndexName(); vespalib::string term = iterator.getTerm(); GeoLocationParser parser; - if (parser.parseOldFormat(term)) { + if (parser.parseNoField(term)) { auto attr_name = PositionDataType::getZCurveFieldName(view); GeoLocationSpec spec{attr_name, parser.getGeoLocation()}; _parsedLocations.push_back(spec); diff --git a/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp b/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp index 06bdb9f69bb..383f6c3d44b 100644 --- a/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp +++ b/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp @@ -26,7 +26,7 @@ parseLocation(const string & location_str) return fefLocations; } GeoLocationParser locationParser; - if (!locationParser.parseOldFormatWithField(location_str)) { + if (!locationParser.parseWithField(location_str)) { LOG(warning, "Location parse error (location: '%s'): %s. Location ignored.", location_str.c_str(), locationParser.getParseError()); return fefLocations; |