[GitHub] [geode] pivotal-jbarrett merged pull request #5266: GEODE-8274: Improve readability of Version comparison.
pivotal-jbarrett merged pull request #5266: URL: https://github.com/apache/geode/pull/5266 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: us...@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #5271: Add Mass Test Run pipeline.
mhansonp commented on a change in pull request #5271: URL: https://github.com/apache/geode/pull/5271#discussion_r441907733 ## File path: ci/pipelines/shared/jinja.variables.yml ## @@ -67,6 +67,7 @@ java_test_versions: metadata: initial_version: 1.14.0-((semver-prerelease-token)).0 + mass_test_run_iterations: 200 Review comment: Basically, 200 gives us a big enough sample size to tell the 1 off from the 2 or more. I think running 100 might be fine. Especially if the report generator script is doing a last 200 check. 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: us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal merged pull request #5258: GEODE-8263: KEEPTTL should fail on SET command
dschneider-pivotal merged pull request #5258: URL: https://github.com/apache/geode/pull/5258 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: us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #5258: GEODE-8263: KEEPTTL should fail on SET command
dschneider-pivotal commented on pull request #5258: URL: https://github.com/apache/geode/pull/5258#issuecomment-645685812 the dunit failure is a known flaky test 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: us...@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
mhansonp commented on a change in pull request #5249: URL: https://github.com/apache/geode/pull/5249#discussion_r441876860 ## File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/RestoreRedundancyPerformer.java ## @@ -112,14 +114,14 @@ public RestoreRedundancyResults perform(Cache cache, RestoreRedundancyRequest op return finalResult; } - // this returns either an Exception or RestoreRedundancyResults + // this returns RestoreRedundancyResults or null based on Review comment: Thanks for your attention to detail. 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: us...@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
mhansonp commented on a change in pull request #5249: URL: https://github.com/apache/geode/pull/5249#discussion_r441876791 ## File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/RestoreRedundancyPerformer.java ## @@ -112,14 +114,14 @@ public RestoreRedundancyResults perform(Cache cache, RestoreRedundancyRequest op return finalResult; } - // this returns either an Exception or RestoreRedundancyResults + // this returns RestoreRedundancyResults or null based on Review comment: Good catch. Task switching is bad. The comment just restates what is visible in the code so I deleted it. 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: us...@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
mhansonp commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r441875211
##
File path:
geode-management/src/main/java/org/apache/geode/management/internal/operation/RegionRedundancyStatusImpl.java
##
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.management.internal.operation;
+
+import org.apache.geode.management.runtime.RegionRedundancyStatus;
+
+/**
+ * result object used by the cms that only needs to be json serializable
+ */
+public class RegionRedundancyStatusImpl implements RegionRedundancyStatus {
+
+ public static final String OUTPUT_STRING =
+ "%s redundancy status: %s. Desired redundancy is %s and actual
redundancy is %s.";
+
+ /**
+ * The name of the region used to create this object.
+ */
+ protected String regionName;
+
+ /**
+ * The configured redundancy of the region used to create this object.
+ */
+ protected int configuredRedundancy;
+
+ /**
+ * The actual redundancy of the region used to create this object at time of
creation.
+ */
+ protected int actualRedundancy;
+
+ /**
+ * The {@link RedundancyStatus} of the region used to create this object at
time of creation.
+ */
+ protected RedundancyStatus status;
+
+ /**
+ * Default constructor used for serialization
+ */
+ public RegionRedundancyStatusImpl() {}
+
+ public RegionRedundancyStatusImpl(int configuredRedundancy, int
actualRedundancy,
Review comment:
Sorry misread that... I agree.
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:
us...@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
mhansonp commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r441874934
##
File path:
geode-management/src/main/java/org/apache/geode/management/internal/operation/RegionRedundancyStatusImpl.java
##
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.management.internal.operation;
+
+import org.apache.geode.management.runtime.RegionRedundancyStatus;
+
+/**
+ * result object used by the cms that only needs to be json serializable
+ */
+public class RegionRedundancyStatusImpl implements RegionRedundancyStatus {
+
+ public static final String OUTPUT_STRING =
+ "%s redundancy status: %s. Desired redundancy is %s and actual
redundancy is %s.";
+
+ /**
+ * The name of the region used to create this object.
+ */
+ protected String regionName;
+
+ /**
+ * The configured redundancy of the region used to create this object.
+ */
+ protected int configuredRedundancy;
+
+ /**
+ * The actual redundancy of the region used to create this object at time of
creation.
+ */
+ protected int actualRedundancy;
+
+ /**
+ * The {@link RedundancyStatus} of the region used to create this object at
time of creation.
+ */
+ protected RedundancyStatus status;
+
+ /**
+ * Default constructor used for serialization
+ */
+ public RegionRedundancyStatusImpl() {}
+
+ public RegionRedundancyStatusImpl(int configuredRedundancy, int
actualRedundancy,
Review comment:
Its necessary for serialization.
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:
us...@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
DonalEvans commented on a change in pull request #5249: URL: https://github.com/apache/geode/pull/5249#discussion_r441873906 ## File path: geode-core/src/main/java/org/apache/geode/management/internal/operation/RestoreRedundancyPerformer.java ## @@ -112,14 +114,14 @@ public RestoreRedundancyResults perform(Cache cache, RestoreRedundancyRequest op return finalResult; } - // this returns either an Exception or RestoreRedundancyResults + // this returns RestoreRedundancyResults or null based on Review comment: Incomplete comment here? 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: us...@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
mhansonp commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r441871215
##
File path:
geode-core/src/test/java/org/apache/geode/management/internal/functions/RestoreRedundancyFunctionTest.java
##
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.functions;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.control.RestoreRedundancyOperation;
+import org.apache.geode.cache.execute.FunctionContext;
+import org.apache.geode.cache.execute.ResultSender;
+import
org.apache.geode.internal.cache.control.SerializableRestoreRedundancyResultsImpl;
+import org.apache.geode.management.operation.RestoreRedundancyRequest;
+import org.apache.geode.management.runtime.RestoreRedundancyResults;
+
+public class RestoreRedundancyFunctionTest {
+ @SuppressWarnings("unchecked")
+ private final FunctionContext mockContext =
mock(FunctionContext.class);
+ private final Cache mockCache = mock(Cache.class, RETURNS_DEEP_STUBS);
+ private final RestoreRedundancyOperation mockOperation =
+ mock(RestoreRedundancyOperation.class, RETURNS_DEEP_STUBS);
+ private final SerializableRestoreRedundancyResultsImpl mockResults =
+ mock(SerializableRestoreRedundancyResultsImpl.class);
+ private final String message = "expected message";
+ private RestoreRedundancyFunction function;
+ private ResultSender resultSender;
+ private ArgumentCaptor
argumentCaptor;
+ private RestoreRedundancyRequest request;
+
+ @Before
+ public void setUp() throws InterruptedException, ExecutionException {
+function = new RestoreRedundancyFunction();
+when(mockContext.getCache()).thenReturn(mockCache);
+request = new RestoreRedundancyRequest();
Review comment:
I think it is reasonable at this point to believe that. It is basically
like a work order being passed in...
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:
us...@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on a change in pull request #5271: Add Mass Test Run pipeline.
onichols-pivotal commented on a change in pull request #5271: URL: https://github.com/apache/geode/pull/5271#discussion_r441869161 ## File path: ci/pipelines/shared/jinja.variables.yml ## @@ -67,6 +67,7 @@ java_test_versions: metadata: initial_version: 1.14.0-((semver-prerelease-token)).0 + mass_test_run_iterations: 200 Review comment: @mhansonp I know there's been discussion in the past about how much value we get out of 100 vs 200. Can you mention the benefits of 200 vs 100? My back-of-the-envelope calculations suggest 200 takes about 4 days, while 100 could be finished in a weekend (and about $900 cheaper, if anyone cares) 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: us...@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
mhansonp commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r441869114
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/functions/RestoreRedundancyFunction.java
##
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.management.internal.functions;
+
+import static
org.apache.geode.management.runtime.RestoreRedundancyResults.Status.ERROR;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.apache.logging.log4j.Logger;
+
+import org.apache.geode.cache.control.RestoreRedundancyOperation;
+import org.apache.geode.cache.execute.FunctionContext;
+import
org.apache.geode.internal.cache.control.SerializableRestoreRedundancyResultsImpl;
+import org.apache.geode.internal.cache.execute.InternalFunction;
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import
org.apache.geode.management.internal.operation.RestoreRedundancyResultsImpl;
+import org.apache.geode.management.operation.RestoreRedundancyRequest;
+
+
+public class RestoreRedundancyFunction implements InternalFunction {
+ private static final Logger logger = LogService.getLogger();
+
+ public static final String ID = RestoreRedundancyFunction.class.getName();
+
+
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ // this would return the RestoreRedundancyResults if successful,
+ // it will return an exception to the caller if status is failure or any
exception happens
+ public void execute(FunctionContext context) {
+Object[] arguments = context.getArguments();
+RestoreRedundancyRequest request = (RestoreRedundancyRequest) arguments[0];
+boolean isStatusCommand = (boolean) arguments[1];
+RestoreRedundancyOperation redundancyOperation =
+
context.getCache().getResourceManager().createRestoreRedundancyOperation();
+Set includeRegionsSet = null;
+if (request.getIncludeRegions() != null) {
+ includeRegionsSet = new HashSet<>(request.getIncludeRegions());
+}
+Set excludeRegionsSet = null;
+if (request.getExcludeRegions() != null) {
+ excludeRegionsSet = new HashSet<>(request.getExcludeRegions());
+}
+redundancyOperation.includeRegions(includeRegionsSet);
+redundancyOperation.excludeRegions(excludeRegionsSet);
+RestoreRedundancyResultsImpl results;
+
+try {
+ if (isStatusCommand) {
+results = (RestoreRedundancyResultsImpl)
redundancyOperation.redundancyStatus();
+ } else {
+
redundancyOperation.shouldReassignPrimaries(request.getReassignPrimaries());
+results = (RestoreRedundancyResultsImpl)
redundancyOperation.start().join();
+ }
+ if (results.getRegionOperationStatus().equals(ERROR)) {
+Exception e = new Exception(results.getRegionOperationMessage());
+throw e;
+ }
+ results.setSuccess(true);
+ results.setStatusMessage("Success"); // MLH change this
+} catch (Exception e) {
+ results =
+ new SerializableRestoreRedundancyResultsImpl();
+ results.setSuccess(false);
+ results.setStatusMessage(e.getMessage());
+}
Review comment:
I don't believe this is required either.
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:
us...@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
mhansonp commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r441868918
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/operation/RestoreRedundancyPerformer.java
##
@@ -0,0 +1,179 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.operation;
+
+import static org.apache.geode.cache.Region.SEPARATOR;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import org.apache.geode.annotations.Immutable;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.execute.Function;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.distributed.DistributedMember;
+import
org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.serialization.Version;
+import org.apache.geode.management.ManagementService;
+import
org.apache.geode.management.internal.functions.RestoreRedundancyFunction;
+import org.apache.geode.management.internal.util.ManagementUtils;
+import org.apache.geode.management.operation.RestoreRedundancyRequest;
+import org.apache.geode.management.runtime.RestoreRedundancyResults;
+
+public class RestoreRedundancyPerformer
+implements OperationPerformer {
+ @Immutable
+ public static final Version ADDED_VERSION = Version.GEODE_1_13_0;
+ public static final String NO_MEMBERS_WITH_VERSION_FOR_REGION =
+ "No members with a version greater than or equal to %s were found for
region %s";
+ public static final String EXCEPTION_MEMBER_MESSAGE = "Exception occurred on
member %s: %s";
+
+ @Override
+ public RestoreRedundancyResults perform(Cache cache,
RestoreRedundancyRequest operation) {
+return perform(cache, operation, false);
+ }
+
+ public RestoreRedundancyResults perform(Cache cache,
RestoreRedundancyRequest operation,
+ boolean checkStatus) {
+List membersForEachRegion = new
ArrayList<>();
+List includedRegionsWithNoMembers = new ArrayList<>();
+
+populateLists(membersForEachRegion, includedRegionsWithNoMembers,
operation.getIncludeRegions(),
+operation.getExcludeRegions(), (InternalCache) cache);
+
+for (RebalanceOperationPerformer.MemberPRInfo prInfo :
membersForEachRegion) {
+ // Filter out any members using older versions of Geode
+ List viableMembers = filterViableMembers(prInfo);
+
+ if (viableMembers.size() != 0) {
+// Update the MemberPRInfo with the viable members
+prInfo.dsMemberList = viableMembers;
+ } else {
+RestoreRedundancyResultsImpl results = new
RestoreRedundancyResultsImpl();
+
results.setStatusMessage(String.format(NO_MEMBERS_WITH_VERSION_FOR_REGION,
+ADDED_VERSION.getName(), prInfo.region));
+results.setSuccess(false);
+return results;
+ }
+}
+
+List functionResults = new ArrayList<>();
+Object[] functionArgs = new Object[] {operation, checkStatus};
+List completedMembers = new ArrayList<>();
+for (RebalanceOperationPerformer.MemberPRInfo memberPRInfo :
membersForEachRegion) {
+ // Check to see if an earlier function execution has already targeted a
member hosting this
+ // region. If one has, there is no point sending a function for this
region as it has already
+ // had redundancy restored
+ if (!Collections.disjoint(completedMembers, memberPRInfo.dsMemberList)) {
+continue;
+ }
+ // Try the function on the first member for this region
+ DistributedMember targetMember = memberPRInfo.dsMemberList.get(0);
+ RestoreRedundancyResults functionResult =
executeFunctionAndGetFunctionResult(
+ new RestoreRedundancyFunction(), functionArgs, targetMember);
+ if (!functionResult.getSuccess()) {
+// Record the error and then give up
+RestoreRedundancyResultsImpl results = new
RestoreRedundancyResultsImpl();
+results.setSuccess(false);
+String errorString =
+String.format(EXCEPTION_MEMBER_MESSAGE,
[GitHub] [geode] mhansonp edited a comment on pull request #5271: Add Mass Test Run pipeline.
mhansonp edited a comment on pull request #5271: URL: https://github.com/apache/geode/pull/5271#issuecomment-645653390 Shall I merge it? It seems like it didn't change much.. 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: us...@infra.apache.org
[GitHub] [geode] rhoughton-pivot commented on a change in pull request #5271: Add Mass Test Run pipeline.
rhoughton-pivot commented on a change in pull request #5271:
URL: https://github.com/apache/geode/pull/5271#discussion_r441861412
##
File path: ci/pipelines/meta/deploy_meta.sh
##
@@ -256,6 +256,7 @@ set +x
if [[ "${GEODE_FORK}" != "${UPSTREAM_FORK}" ]]; then
echo "Disabling unnecessary jobs for forks."
pauseJobs ${META_PIPELINE} set-reaper-pipeline
+ pauseJobs ${META_PIPELINE} set-mass-test-run-pipeline
Review comment:
done
##
File path: ci/bin/concourse_job_performance.py
##
@@ -58,7 +60,7 @@ def main(url, team, pipeline, job, number_of_builds,
authorization_cookie, threa
builds = get_builds_summary_sheet(url, team, pipeline, job,
number_of_builds+10, authorization_cookie)
build_to_examine = get_builds_to_examine(builds, number_of_builds)
-expected_failed_builds = [int(b['name']) for b in build_to_examine if
b['status'] == 'failed']
+expected_failed_builds = [int(b['name']) for b in build_to_examine if
b['status'] in ['failed', 'succeeded']]
Review comment:
done
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:
us...@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
DonalEvans commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r441860267
##
File path:
geode-core/src/test/java/org/apache/geode/management/internal/functions/RestoreRedundancyFunctionTest.java
##
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.functions;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.control.RestoreRedundancyOperation;
+import org.apache.geode.cache.execute.FunctionContext;
+import org.apache.geode.cache.execute.ResultSender;
+import
org.apache.geode.internal.cache.control.SerializableRestoreRedundancyResultsImpl;
+import org.apache.geode.management.operation.RestoreRedundancyRequest;
+import org.apache.geode.management.runtime.RestoreRedundancyResults;
+
+public class RestoreRedundancyFunctionTest {
+ @SuppressWarnings("unchecked")
+ private final FunctionContext mockContext =
mock(FunctionContext.class);
+ private final Cache mockCache = mock(Cache.class, RETURNS_DEEP_STUBS);
+ private final RestoreRedundancyOperation mockOperation =
+ mock(RestoreRedundancyOperation.class, RETURNS_DEEP_STUBS);
+ private final SerializableRestoreRedundancyResultsImpl mockResults =
+ mock(SerializableRestoreRedundancyResultsImpl.class);
+ private final String message = "expected message";
+ private RestoreRedundancyFunction function;
+ private ResultSender resultSender;
+ private ArgumentCaptor
argumentCaptor;
+ private RestoreRedundancyRequest request;
+
+ @Before
+ public void setUp() throws InterruptedException, ExecutionException {
+function = new RestoreRedundancyFunction();
+when(mockContext.getCache()).thenReturn(mockCache);
+request = new RestoreRedundancyRequest();
Review comment:
If we can be reasonably sure that this will remain true, and that no
additional logic will be added to `RestoreRedundancyRequest` in the future,
then this is fine to leave as it is.
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:
us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal opened a new pull request #5272: GEODE-8276: fix KEYS command to handle non-ASCII keys
dschneider-pivotal opened a new pull request #5272: URL: https://github.com/apache/geode/pull/5272 Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, check Concourse for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. 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: us...@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on a change in pull request #5271: Add Mass Test Run pipeline.
onichols-pivotal commented on a change in pull request #5271:
URL: https://github.com/apache/geode/pull/5271#discussion_r441858177
##
File path: ci/pipelines/meta/deploy_meta.sh
##
@@ -256,6 +256,7 @@ set +x
if [[ "${GEODE_FORK}" != "${UPSTREAM_FORK}" ]]; then
echo "Disabling unnecessary jobs for forks."
pauseJobs ${META_PIPELINE} set-reaper-pipeline
+ pauseJobs ${META_PIPELINE} set-mass-test-run-pipeline
Review comment:
Please also add `enableFeature mass-test-run` to
`ci/pipelines/meta/deploy_meta.sh` line ~285 just below where `pr` is enabled
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:
us...@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
mhansonp commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r441858350
##
File path:
geode-core/src/test/java/org/apache/geode/management/internal/functions/RestoreRedundancyFunctionTest.java
##
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.functions;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.control.RestoreRedundancyOperation;
+import org.apache.geode.cache.execute.FunctionContext;
+import org.apache.geode.cache.execute.ResultSender;
+import
org.apache.geode.internal.cache.control.SerializableRestoreRedundancyResultsImpl;
+import org.apache.geode.management.operation.RestoreRedundancyRequest;
+import org.apache.geode.management.runtime.RestoreRedundancyResults;
+
+public class RestoreRedundancyFunctionTest {
+ @SuppressWarnings("unchecked")
+ private final FunctionContext mockContext =
mock(FunctionContext.class);
+ private final Cache mockCache = mock(Cache.class, RETURNS_DEEP_STUBS);
+ private final RestoreRedundancyOperation mockOperation =
+ mock(RestoreRedundancyOperation.class, RETURNS_DEEP_STUBS);
+ private final SerializableRestoreRedundancyResultsImpl mockResults =
+ mock(SerializableRestoreRedundancyResultsImpl.class);
+ private final String message = "expected message";
+ private RestoreRedundancyFunction function;
+ private ResultSender resultSender;
+ private ArgumentCaptor
argumentCaptor;
+ private RestoreRedundancyRequest request;
+
+ @Before
+ public void setUp() throws InterruptedException, ExecutionException {
+function = new RestoreRedundancyFunction();
+when(mockContext.getCache()).thenReturn(mockCache);
+request = new RestoreRedundancyRequest();
Review comment:
I was looking at that and it does seem like its possible, but it doesn't
seem worth it since the request is just a flat data class and doesn't do much
in it.
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:
us...@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #5271: Add Mass Test Run pipeline.
mhansonp commented on a change in pull request #5271: URL: https://github.com/apache/geode/pull/5271#discussion_r441853889 ## File path: ci/bin/concourse_job_performance.py ## @@ -58,7 +60,7 @@ def main(url, team, pipeline, job, number_of_builds, authorization_cookie, threa builds = get_builds_summary_sheet(url, team, pipeline, job, number_of_builds+10, authorization_cookie) build_to_examine = get_builds_to_examine(builds, number_of_builds) -expected_failed_builds = [int(b['name']) for b in build_to_examine if b['status'] == 'failed'] +expected_failed_builds = [int(b['name']) for b in build_to_examine if b['status'] in ['failed', 'succeeded']] Review comment: You might want to rename the variable if now you are looking at failed and succeeded builds. 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: us...@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5270: GEODE-6150: added test
lgtm-com[bot] commented on pull request #5270: URL: https://github.com/apache/geode/pull/5270#issuecomment-645638238 This pull request **introduces 8 alerts** and **fixes 2** when merging 5ee88eb8a6ae6cf2113bfefd8d9d6f17dd6e3df9 into d1e857de0d32742f65768e246c49db88ba90cc47 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-0f8ed490f8c9de4d3f88b474712741c005d50a9b) **new alerts:** * 3 for Potential output resource leak * 3 for Dereferenced variable may be null * 2 for Potential input resource leak **fixed alerts:** * 2 for Potential input resource leak 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: us...@infra.apache.org
[GitHub] [geode] rhoughton-pivot opened a new pull request #5271: Add Mass Test Run pipeline.
rhoughton-pivot opened a new pull request #5271: URL: https://github.com/apache/geode/pull/5271 This commit includes: * Creating a new test run pipeline that runs the distributed test multiple number of times. * Update the meta pipeline to add mass test run pipeline. * Logs into concourse using Vault creds to get fly token. * Report-generation job runs often, but only generates a report every N iterations. * Use 'try' around test job, so we can trigger on it's completion state Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [X] Is your initial contribution a single, squashed commit? - [X] Does `gradlew build` run cleanly? - [n/a] Have you written or updated unit tests to verify your changes? - [n/a] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, check Concourse for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. 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: us...@infra.apache.org
[GitHub] [geode] mivanac opened a new pull request #5270: GEODE-6150: added test
mivanac opened a new pull request #5270: URL: https://github.com/apache/geode/pull/5270 WIP: Do not review Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [*] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [*] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [*] Is your initial contribution a single, squashed commit? - [*] Does `gradlew build` run cleanly? - [*] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, check Concourse for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. 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: us...@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441827458
##
File path: cppcache/src/PdxTypeRegistry.hpp
##
@@ -53,7 +53,20 @@ typedef std::unordered_map,
dereference_hash>,
dereference_equal_to>>
PreservedHashMap;
-typedef std::map, int32_t, PdxTypeLessThan>
+
+struct PdxTypeHashCode {
+ std::size_t operator()(std::shared_ptr const& pdx) const {
+return pdx ? pdx->hashcode() : 0;
+ }
+};
+
+struct PdxTypeEqualCmp {
+ bool operator()(std::shared_ptr const& first,
std::shared_ptr const& second) const{
+return first->hashcode() == second->hashcode();
Review comment:
Sorry, I read again, I got your point, two objects could generate the
same hash being different :)
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:
us...@infra.apache.org
[GitHub] [geode] aaronlindsey commented on pull request #5236: GEODE-8241: Locator observes locator-wait-time
aaronlindsey commented on pull request #5236: URL: https://github.com/apache/geode/pull/5236#issuecomment-645616125 I've been running this test in the background while I work and so far it has passed 300x in a row without failing. I will continue letting it run until it reaches 1000 iterations. 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: us...@infra.apache.org
[GitHub] [geode] aaronlindsey commented on a change in pull request #5236: GEODE-8241: Locator observes locator-wait-time
aaronlindsey commented on a change in pull request #5236:
URL: https://github.com/apache/geode/pull/5236#discussion_r441823437
##
File path:
geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/MembershipIntegrationTest.java
##
@@ -172,6 +181,92 @@ public void
secondMembershipCanJoinUsingTheSecondLocatorToStart()
stop(locator2, locator1);
}
+ @Test
+ public void secondMembershipPausesForLocatorWaitTime()
+ throws IOException, MemberStartupException {
+
+/*
+ * Start a locator for the coordinator (membership) so we have a port for
it.
+ *
+ * Its locator-wait-time is set to 0 so it eventually (soon after
membership is started) forms a
+ * distributed system and becomes a coordinator.
+ */
+
+final MembershipLocator coordinatorLocator =
createLocator(0);
+coordinatorLocator.start();
+final int coordinatorLocatorPort = coordinatorLocator.getPort();
+
+final Membership coordinatorMembership =
+createMembership(coordinatorLocator, coordinatorLocatorPort);
+
+/*
+ * We have not even started the membership yet — connection attempts will
certainly fail until
+ * we do. This is a bit like the locator (host) not being present in DNS
(yet).
+ */
+
+/*
+ * Start a second locator and membership trying to join via the
coordinator (membership) that
+ * hasn't yet started behind the port.
+ *
+ * Set its locator-wait-time so it'll not become a coordinator right away,
allowing time for the
+ * other member to start and become a coordinator.
+ *
+ * Calculate the locator-wait-time to be greater than the minimum wait
time for connecting to a
+ * locator.
+ */
+
+final MembershipLocator lateJoiningLocator =
createLocator(0);
+lateJoiningLocator.start();
+final int lateJoiningLocatorPort = lateJoiningLocator.getPort();
+
+final int[] lateJoiningMembershipLocatorPorts =
+new int[] {coordinatorLocatorPort, lateJoiningLocatorPort};
+
+final Duration minimumJoinWaitTime = Duration
+.ofMillis(JOIN_RETRY_SLEEP + FIND_LOCATOR_RETRY_SLEEP) // amount of
sleep time per retry
+.multipliedBy(lateJoiningMembershipLocatorPorts.length * 2); //
expected number of retries
+final int locatorWaitTime = (int) (3 * minimumJoinWaitTime.getSeconds());
+
+final MembershipConfig lateJoiningMembershipConfig =
+createMembershipConfig(true, locatorWaitTime,
lateJoiningMembershipLocatorPorts);
+final Membership lateJoiningMembership =
+createMembership(lateJoiningMembershipConfig, lateJoiningLocator);
+
+CompletableFuture lateJoiningMembershipStartup =
executorServiceRule.runAsync(() -> {
+ try {
+start(lateJoiningMembership);
+ } catch (MemberStartupException e) {
+throw new RuntimeException(e);
+ }
+});
+
+/*
+ * Now start the coordinator (membership), after waiting longer than the
minimum wait time for
+ * connecting to a locator but shorter than the locator-wait-time.
+ */
+
+CompletableFuture coordinatorMembershipStartup =
executorServiceRule.runAsync(() -> {
+ try {
+Thread.sleep(2 * minimumJoinWaitTime.toMillis());
Review comment:
I added a comment to the code in the latest commit.
##
File path:
geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/MembershipIntegrationTest.java
##
@@ -172,6 +181,92 @@ public void
secondMembershipCanJoinUsingTheSecondLocatorToStart()
stop(locator2, locator1);
}
+ @Test
+ public void secondMembershipPausesForLocatorWaitTime()
+ throws IOException, MemberStartupException {
+
+/*
+ * Start a locator for the coordinator (membership) so we have a port for
it.
+ *
+ * Its locator-wait-time is set to 0 so it eventually (soon after
membership is started) forms a
+ * distributed system and becomes a coordinator.
+ */
+
+final MembershipLocator coordinatorLocator =
createLocator(0);
+coordinatorLocator.start();
+final int coordinatorLocatorPort = coordinatorLocator.getPort();
+
+final Membership coordinatorMembership =
+createMembership(coordinatorLocator, coordinatorLocatorPort);
+
+/*
+ * We have not even started the membership yet — connection attempts will
certainly fail until
+ * we do. This is a bit like the locator (host) not being present in DNS
(yet).
+ */
+
+/*
+ * Start a second locator and membership trying to join via the
coordinator (membership) that
+ * hasn't yet started behind the port.
+ *
+ * Set its locator-wait-time so it'll not become a coordinator right away,
allowing time for the
+ * other member to start and become a coordinator.
+ *
+ * Calculate the locator-wait-time to be greater than the minimum wait
time for connecting to a
+ * locator.
+ */
+
+final
[GitHub] [geode-native] alb3rtobr commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441823764
##
File path: cppcache/src/PdxTypeRegistry.hpp
##
@@ -53,7 +53,20 @@ typedef std::unordered_map,
dereference_hash>,
dereference_equal_to>>
PreservedHashMap;
-typedef std::map, int32_t, PdxTypeLessThan>
+
+struct PdxTypeHashCode {
+ std::size_t operator()(std::shared_ptr const& pdx) const {
+return pdx ? pdx->hashcode() : 0;
+ }
+};
+
+struct PdxTypeEqualCmp {
+ bool operator()(std::shared_ptr const& first,
std::shared_ptr const& second) const{
+return first->hashcode() == second->hashcode();
Review comment:
Agree, but I dont want to check if the objects are equals, I just want
to check if two `PdxType` objects have the same hash.
Two `PdxType` objects should be equals if they have the same classname and
their fields have the same types, names and values.
And two keys of the `pdxTypeToTypeId` map will be equals if they have the
same hash, which means they have the same classname, and their fields have the
same types and names (values are not checked here).
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:
us...@infra.apache.org
[GitHub] [geode] aaronlindsey commented on a change in pull request #5236: GEODE-8241: Locator observes locator-wait-time
aaronlindsey commented on a change in pull request #5236:
URL: https://github.com/apache/geode/pull/5236#discussion_r441823755
##
File path:
geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/MembershipIntegrationTest.java
##
@@ -172,6 +181,92 @@ public void
secondMembershipCanJoinUsingTheSecondLocatorToStart()
stop(locator2, locator1);
}
+ @Test
+ public void secondMembershipPausesForLocatorWaitTime()
+ throws IOException, MemberStartupException {
+
+/*
+ * Start a locator for the coordinator (membership) so we have a port for
it.
+ *
+ * Its locator-wait-time is set to 0 so it eventually (soon after
membership is started) forms a
+ * distributed system and becomes a coordinator.
+ */
+
+final MembershipLocator coordinatorLocator =
createLocator(0);
+coordinatorLocator.start();
+final int coordinatorLocatorPort = coordinatorLocator.getPort();
+
+final Membership coordinatorMembership =
+createMembership(coordinatorLocator, coordinatorLocatorPort);
+
+/*
+ * We have not even started the membership yet — connection attempts will
certainly fail until
+ * we do. This is a bit like the locator (host) not being present in DNS
(yet).
+ */
+
+/*
+ * Start a second locator and membership trying to join via the
coordinator (membership) that
+ * hasn't yet started behind the port.
+ *
+ * Set its locator-wait-time so it'll not become a coordinator right away,
allowing time for the
+ * other member to start and become a coordinator.
+ *
+ * Calculate the locator-wait-time to be greater than the minimum wait
time for connecting to a
+ * locator.
+ */
+
+final MembershipLocator lateJoiningLocator =
createLocator(0);
+lateJoiningLocator.start();
+final int lateJoiningLocatorPort = lateJoiningLocator.getPort();
+
+final int[] lateJoiningMembershipLocatorPorts =
+new int[] {coordinatorLocatorPort, lateJoiningLocatorPort};
+
+final Duration minimumJoinWaitTime = Duration
+.ofMillis(JOIN_RETRY_SLEEP + FIND_LOCATOR_RETRY_SLEEP) // amount of
sleep time per retry
+.multipliedBy(lateJoiningMembershipLocatorPorts.length * 2); //
expected number of retries
Review comment:
I extracted a method for calculating the minimum number of retries
before becoming coordinator in the latest commit.
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:
us...@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey merged pull request #618: GEODE-8246: Enforce No Shadow as error
pdxcodemonkey merged pull request #618: URL: https://github.com/apache/geode-native/pull/618 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: us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441813608
##
File path: cppcache/src/PdxType.cpp
##
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash strHash;
Review comment:
See the bottom of cppcache/include/geode/CacheableKey.hpp for how we
export `std::hash` and you can do the same thing for
`std::hash`.
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:
us...@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
pdxcodemonkey commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441812743
##
File path: cppcache/src/PdxTypeRegistry.hpp
##
@@ -53,7 +53,20 @@ typedef std::unordered_map,
dereference_hash>,
dereference_equal_to>>
PreservedHashMap;
-typedef std::map, int32_t, PdxTypeLessThan>
+
+struct PdxTypeHashCode {
+ std::size_t operator()(std::shared_ptr const& pdx) const {
+return pdx ? pdx->hashcode() : 0;
+ }
+};
+
+struct PdxTypeEqualCmp {
+ bool operator()(std::shared_ptr const& first,
std::shared_ptr const& second) const{
+return first->hashcode() == second->hashcode();
+ }
+};
+
+typedef std::unordered_map, int32_t, PdxTypeHashCode,
PdxTypeEqualCmp>
Review comment:
+1, I like this!
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:
us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request #611: GEODE-8213: lock-free SerializationRegistry
pivotal-jbarrett commented on pull request #611: URL: https://github.com/apache/geode-native/pull/611#issuecomment-645604306 I think we should close this PR and revisit if after the fixes in https://github.com/apache/geode-native/pull/619 to be merged. After that I think It might just be safe to start by replacing the spinlock with just a plain old mutex to reduce the CPU load. It may not bench as good but solves the issue of CPU load under contention. 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: us...@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441808727
##
File path: cppcache/src/PdxType.cpp
##
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash strHash;
Review comment:
> In fact I would just create a specialization of `std::hash`.
I will check this
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:
us...@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441807945
##
File path: cppcache/src/PdxType.cpp
##
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash strHash;
Review comment:
> Just a thought here. Is it necessary to hash like Java client does in
this case?
No, we just need a hash function to allow identify `PdxType` objects that
will be assigned the same id by the server. We dont need to know the hash
generated by the server.
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:
us...@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441807945
##
File path: cppcache/src/PdxType.cpp
##
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash strHash;
Review comment:
> Just a thought here. Is it necessary to hash like Java client does in
this case?
No, we just need a hash function to allow identify `PdxType` object that
will be assigned the same id by the server. We dont need to know the hash
generated by the server.
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:
us...@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441807945
##
File path: cppcache/src/PdxType.cpp
##
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash strHash;
Review comment:
> Just a thought here. Is it necessary to hash like Java client does in
this case?
No, we just need a hash function to allow identify `PdxType` object that
will be assigned the same id by the server. We dont need to know the hash
generated by the server.
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:
us...@infra.apache.org
[GitHub] [geode] Bill opened a new pull request #5269: new failing unit test for unknown version
Bill opened a new pull request #5269: URL: https://github.com/apache/geode/pull/5269 [GEODE-8240](https://issues.apache.org/jira/browse/GEODE-8240) Just getting started here. **Not ready for review yet** ### For all changes: - [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [x] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [x] Have you written or updated unit tests to verify your changes? 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: us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441805042
##
File path: cppcache/src/PdxType.cpp
##
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash strHash;
+ auto result=strHash(this->m_className);
+
+ for (std::vector>::iterator it =
+ m_pdxFieldTypes->begin();
+ it != m_pdxFieldTypes->end(); ++it) {
+auto pdxPtr = *it;
+result = result ^ (strHash(pdxPtr->getClassName()) << 1 );
Review comment:
> > Just to be clear, I didn't mean to suggest we not hash in the
classname at all, just outside the for loop. Also for what it's worth, even
though using classname is still correct, the only instance of PDX we're aware
of that has this problem, i.e. the classname isn't unique, is for JSON
instances, where classname is always "__GEMFIRE_JSON".
>
> oh, I see the confusion: the classname its outside the for loop (line
573). The other classnames you are refering to are the classnames of each
field. If we dont include the name of the field and its classname, the hash
will be the same for an object with a bool called `foo` and for an object with
a string called `foo`.
I don't think the Java implementation will distinguish one OBJECT from
anther OBJECT . I think you just need to has the field name and type and ignore
the class name.
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:
us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441799345
##
File path: cppcache/src/PdxType.cpp
##
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash strHash;
Review comment:
Just a thought here. Is it necessary to hash like Java client does in
this case? If the `PdxType::hashcode()` is expected to hash similarly to that
of Java version then changes are needed here. If not then I wouldn't try to
implement the `CacheableKey::hashcode` method here and do something more
compatible with C++, like `std::hash` which produces a `size_t`. In fact I
would just create a specialization of `std::hash`.
##
File path: cppcache/src/PdxTypeRegistry.hpp
##
@@ -53,7 +53,20 @@ typedef std::unordered_map,
dereference_hash>,
dereference_equal_to>>
PreservedHashMap;
-typedef std::map, int32_t, PdxTypeLessThan>
+
+struct PdxTypeHashCode {
+ std::size_t operator()(std::shared_ptr const& pdx) const {
+return pdx ? pdx->hashcode() : 0;
+ }
+};
+
+struct PdxTypeEqualCmp {
+ bool operator()(std::shared_ptr const& first,
std::shared_ptr const& second) const{
+return first->hashcode() == second->hashcode();
Review comment:
Equality of hashcode does not guarantee equality of the object. You
should just implement `PdxType::operator==`.
##
File path: cppcache/src/PdxTypeRegistry.hpp
##
@@ -53,7 +53,20 @@ typedef std::unordered_map,
dereference_hash>,
dereference_equal_to>>
PreservedHashMap;
-typedef std::map, int32_t, PdxTypeLessThan>
+
+struct PdxTypeHashCode {
+ std::size_t operator()(std::shared_ptr const& pdx) const {
+return pdx ? pdx->hashcode() : 0;
+ }
+};
+
+struct PdxTypeEqualCmp {
+ bool operator()(std::shared_ptr const& first,
std::shared_ptr const& second) const{
+return first->hashcode() == second->hashcode();
+ }
+};
+
+typedef std::unordered_map, int32_t, PdxTypeHashCode,
PdxTypeEqualCmp>
Review comment:
If you implement `std::hash` and `PdxType::operator==` then you
can use the `dereference_hash>` and
`dereference_equal_to>>` here.
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:
us...@infra.apache.org
[GitHub] [geode] davebarnes97 commented on pull request #5139: GEODE-8149: New parameter and property introduced
davebarnes97 commented on pull request #5139: URL: https://github.com/apache/geode/pull/5139#issuecomment-645594536 > OK, I will create sub-ticket to handle User Guide docs. I'm not finding the sub-ticket for docs. Please provide the ticket number - thanks. 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: us...@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441801421
##
File path: cppcache/src/PdxType.cpp
##
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash strHash;
+ auto result=strHash(this->m_className);
+
+ for (std::vector>::iterator it =
+ m_pdxFieldTypes->begin();
+ it != m_pdxFieldTypes->end(); ++it) {
+auto pdxPtr = *it;
+result = result ^ (strHash(pdxPtr->getClassName()) << 1 );
Review comment:
> It would be good to have a unit test that shows the hashcode doesn't
change if you iterate the fields in a different order...
sure, Im working on that :)
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:
us...@infra.apache.org
[GitHub] [geode] kirklund opened a new pull request #5268: GEODE-8250: Create new custom log config acceptance tests
kirklund opened a new pull request #5268: URL: https://github.com/apache/geode/pull/5268 New tests to discover and verify that use of custom log configs works when using GFSH to start Locator and Server. 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: us...@infra.apache.org
[GitHub] [geode] kirklund opened a new pull request #5267: GEODE-8273: Cleanup GfshExecution and GfshScript
kirklund opened a new pull request #5267: URL: https://github.com/apache/geode/pull/5267 * Use GeodeAwaitility timeout instead of hardcoded timeouts. * Use just one source for Charset. * Minor formatting changes. 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: us...@infra.apache.org
[GitHub] [geode] pivotal-jbarrett opened a new pull request #5266: GEODE-8274: Improve readability of Version comparison.
pivotal-jbarrett opened a new pull request #5266: URL: https://github.com/apache/geode/pull/5266 Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, check Concourse for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. 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: us...@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #5249: Refactor Restore Redundancy Command
DonalEvans commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r441782824
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/operation/RestoreRedundancyPerformer.java
##
@@ -0,0 +1,179 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.operation;
+
+import static org.apache.geode.cache.Region.SEPARATOR;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import org.apache.geode.annotations.Immutable;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.execute.Function;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.distributed.DistributedMember;
+import
org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.serialization.Version;
+import org.apache.geode.management.ManagementService;
+import
org.apache.geode.management.internal.functions.RestoreRedundancyFunction;
+import org.apache.geode.management.internal.util.ManagementUtils;
+import org.apache.geode.management.operation.RestoreRedundancyRequest;
+import org.apache.geode.management.runtime.RestoreRedundancyResults;
+
+public class RestoreRedundancyPerformer
+implements OperationPerformer {
+ @Immutable
+ public static final Version ADDED_VERSION = Version.GEODE_1_13_0;
+ public static final String NO_MEMBERS_WITH_VERSION_FOR_REGION =
+ "No members with a version greater than or equal to %s were found for
region %s";
+ public static final String EXCEPTION_MEMBER_MESSAGE = "Exception occurred on
member %s: %s";
+
+ @Override
+ public RestoreRedundancyResults perform(Cache cache,
RestoreRedundancyRequest operation) {
+return perform(cache, operation, false);
+ }
+
+ public RestoreRedundancyResults perform(Cache cache,
RestoreRedundancyRequest operation,
+ boolean checkStatus) {
+List membersForEachRegion = new
ArrayList<>();
+List includedRegionsWithNoMembers = new ArrayList<>();
+
+populateLists(membersForEachRegion, includedRegionsWithNoMembers,
operation.getIncludeRegions(),
+operation.getExcludeRegions(), (InternalCache) cache);
+
+for (RebalanceOperationPerformer.MemberPRInfo prInfo :
membersForEachRegion) {
+ // Filter out any members using older versions of Geode
+ List viableMembers = filterViableMembers(prInfo);
+
+ if (viableMembers.size() != 0) {
+// Update the MemberPRInfo with the viable members
+prInfo.dsMemberList = viableMembers;
+ } else {
+RestoreRedundancyResultsImpl results = new
RestoreRedundancyResultsImpl();
+
results.setStatusMessage(String.format(NO_MEMBERS_WITH_VERSION_FOR_REGION,
+ADDED_VERSION.getName(), prInfo.region));
+results.setSuccess(false);
+return results;
+ }
+}
+
+List functionResults = new ArrayList<>();
+Object[] functionArgs = new Object[] {operation, checkStatus};
+List completedMembers = new ArrayList<>();
+for (RebalanceOperationPerformer.MemberPRInfo memberPRInfo :
membersForEachRegion) {
+ // Check to see if an earlier function execution has already targeted a
member hosting this
+ // region. If one has, there is no point sending a function for this
region as it has already
+ // had redundancy restored
+ if (!Collections.disjoint(completedMembers, memberPRInfo.dsMemberList)) {
+continue;
+ }
+ // Try the function on the first member for this region
+ DistributedMember targetMember = memberPRInfo.dsMemberList.get(0);
+ RestoreRedundancyResults functionResult =
executeFunctionAndGetFunctionResult(
+ new RestoreRedundancyFunction(), functionArgs, targetMember);
+ if (!functionResult.getSuccess()) {
+// Record the error and then give up
+RestoreRedundancyResultsImpl results = new
RestoreRedundancyResultsImpl();
+results.setSuccess(false);
+String errorString =
+String.format(EXCEPTION_MEMBER_MESSAGE,
[GitHub] [geode] bschuchardt opened a new pull request #5265: work in progress
bschuchardt opened a new pull request #5265: URL: https://github.com/apache/geode/pull/5265 Use of SSLEngine with NIO has proven to be less robust than using "old-io" SSLSockets. This draft PR reintroduces SSLSockets into cluster communications for performance and functional testing. The structure of the org.apache.geode.internal.tcp package is left intact. None of the old code including Connection.runNioReader() has been brought back. Code differences between use of NIO and OIO are minimal and are at the socket-read/socket-write level. Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, check Concourse for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. 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: us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal merged pull request #5263: GEODE-8270: Reorganize Redis API for Geode test packages to match main package structure
dschneider-pivotal merged pull request #5263: URL: https://github.com/apache/geode/pull/5263 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: us...@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
pdxcodemonkey commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441751119
##
File path: cppcache/src/PdxType.cpp
##
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash strHash;
+ auto result=strHash(this->m_className);
+
+ for (std::vector>::iterator it =
+ m_pdxFieldTypes->begin();
+ it != m_pdxFieldTypes->end(); ++it) {
+auto pdxPtr = *it;
+result = result ^ (strHash(pdxPtr->getClassName()) << 1 );
Review comment:
It would be good to have a unit test that shows the hashcode doesn't
change if you iterate the fields in a different order...
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:
us...@infra.apache.org
[GitHub] [geode] aaronlindsey commented on pull request #5236: GEODE-8241: Locator observes locator-wait-time
aaronlindsey commented on pull request #5236: URL: https://github.com/apache/geode/pull/5236#issuecomment-645549135 @Bill what do you think of mocking out calls to `System.currentTimeMillis()` in `GMSJoinLeave`? Allowing the test to control the clock might allow a more deterministic test. I'm not sure if it would work, though, because there are a lot of usages of `System.currentTimeMillis()` in `GMSJoinLeave` that our test doesn't care about which would also be affected. 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: us...@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
pdxcodemonkey commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441749254
##
File path: cppcache/src/PdxType.cpp
##
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash strHash;
+ auto result=strHash(this->m_className);
+
+ for (std::vector>::iterator it =
+ m_pdxFieldTypes->begin();
+ it != m_pdxFieldTypes->end(); ++it) {
+auto pdxPtr = *it;
+result = result ^ (strHash(pdxPtr->getClassName()) << 1 );
Review comment:
Just to be clear, I didn't mean to suggest we not hash in the classname
at all, just outside the for loop. Also for what it's worth, even though using
classname is still correct, the only instance of PDX we're aware of that has
this problem, i.e. the classname isn't unique, is for JSON instances, where
classname is always "__GEMFIRE_JSON".
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:
us...@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #5249: Refactor Restore Redundancy Command
mhansonp commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r441733613
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/operation/RestoreRedundancyPerformer.java
##
@@ -0,0 +1,179 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.operation;
+
+import static org.apache.geode.cache.Region.SEPARATOR;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import org.apache.geode.annotations.Immutable;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.execute.Function;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.distributed.DistributedMember;
+import
org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.serialization.Version;
+import org.apache.geode.management.ManagementService;
+import
org.apache.geode.management.internal.functions.RestoreRedundancyFunction;
+import org.apache.geode.management.internal.util.ManagementUtils;
+import org.apache.geode.management.operation.RestoreRedundancyRequest;
+import org.apache.geode.management.runtime.RestoreRedundancyResults;
+
+public class RestoreRedundancyPerformer
+implements OperationPerformer {
+ @Immutable
+ public static final Version ADDED_VERSION = Version.GEODE_1_13_0;
+ public static final String NO_MEMBERS_WITH_VERSION_FOR_REGION =
+ "No members with a version greater than or equal to %s were found for
region %s";
+ public static final String EXCEPTION_MEMBER_MESSAGE = "Exception occurred on
member %s: %s";
+
+ @Override
+ public RestoreRedundancyResults perform(Cache cache,
RestoreRedundancyRequest operation) {
+return perform(cache, operation, false);
+ }
+
+ public RestoreRedundancyResults perform(Cache cache,
RestoreRedundancyRequest operation,
+ boolean checkStatus) {
+List membersForEachRegion = new
ArrayList<>();
+List includedRegionsWithNoMembers = new ArrayList<>();
+
+populateLists(membersForEachRegion, includedRegionsWithNoMembers,
operation.getIncludeRegions(),
+operation.getExcludeRegions(), (InternalCache) cache);
+
+for (RebalanceOperationPerformer.MemberPRInfo prInfo :
membersForEachRegion) {
+ // Filter out any members using older versions of Geode
+ List viableMembers = filterViableMembers(prInfo);
+
+ if (viableMembers.size() != 0) {
+// Update the MemberPRInfo with the viable members
+prInfo.dsMemberList = viableMembers;
+ } else {
+RestoreRedundancyResultsImpl results = new
RestoreRedundancyResultsImpl();
+
results.setStatusMessage(String.format(NO_MEMBERS_WITH_VERSION_FOR_REGION,
+ADDED_VERSION.getName(), prInfo.region));
+results.setSuccess(false);
+return results;
+ }
+}
+
+List functionResults = new ArrayList<>();
+Object[] functionArgs = new Object[] {operation, checkStatus};
+List completedMembers = new ArrayList<>();
+for (RebalanceOperationPerformer.MemberPRInfo memberPRInfo :
membersForEachRegion) {
+ // Check to see if an earlier function execution has already targeted a
member hosting this
+ // region. If one has, there is no point sending a function for this
region as it has already
+ // had redundancy restored
+ if (!Collections.disjoint(completedMembers, memberPRInfo.dsMemberList)) {
+continue;
+ }
+ // Try the function on the first member for this region
+ DistributedMember targetMember = memberPRInfo.dsMemberList.get(0);
+ RestoreRedundancyResults functionResult =
executeFunctionAndGetFunctionResult(
+ new RestoreRedundancyFunction(), functionArgs, targetMember);
+ if (!functionResult.getSuccess()) {
+// Record the error and then give up
+RestoreRedundancyResultsImpl results = new
RestoreRedundancyResultsImpl();
+results.setSuccess(false);
+String errorString =
+String.format(EXCEPTION_MEMBER_MESSAGE,
[GitHub] [geode] albertogpz commented on pull request #5257: GEODE-8251: make sure Configuration can be deserialized post 1.12.
albertogpz commented on pull request #5257: URL: https://github.com/apache/geode/pull/5257#issuecomment-645524694 > > > > What's the point for these changes? > > > > I have ran the test case without them and it passes. > > > > Also, I have searched in the Geode code for the use of this file and have only found it in `ObjectInputStreamFilterWrapper.java`. From what I see there, only the first part of every line, the class name, is used. > > > > What am I missing? > > > > > > > > > the previous version of Deployment has variable name "jarFileName" instead of "fileName", when we are deserializing a previous version bytes, if the variable name doesn't match, it won't take the value. We should have a test to verify that, but I saw the behavior in the debugger. > > > > > > I understand that. What I do not get are the changes in the > > > > What's the point for these changes? > > > > I have ran the test case without them and it passes. > > > > Also, I have searched in the Geode code for the use of this file and have only found it in `ObjectInputStreamFilterWrapper.java`. From what I see there, only the first part of every line, the class name, is used. > > > > What am I missing? > > > > > > > > > the previous version of Deployment has variable name "jarFileName" instead of "fileName", when we are deserializing a previous version bytes, if the variable name doesn't match, it won't take the value. We should have a test to verify that, but I saw the behavior in the debugger. > > > > > > I understand the change in the members of the class. What I do not get is why it needs to be reflected in sanctioned-geode-management-serializables.txt. > > Who is accessing that file? > > the `AnalyzeManagementSerializablesJUnitTest` is making sure that we don't make random changes in those serialized classes. Every change in the sanction list poses a potential upgrade issue. So the fact that the test is broken will make the developer think twice about the changes he/she is making to the class. I see now. Thanks for the clarification. 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: us...@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441715962
##
File path: cppcache/src/PdxType.cpp
##
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash strHash;
+ auto result=strHash(this->m_className);
+
+ for (std::vector>::iterator it =
+ m_pdxFieldTypes->begin();
+ it != m_pdxFieldTypes->end(); ++it) {
+auto pdxPtr = *it;
+result = result ^ (strHash(pdxPtr->getClassName()) << 1 );
Review comment:
> Do we need to xor in the classname each time through the loop? It
seems like just using the field names in here would be more appropriate
I added it because I thought in the case of two different classes having the
same fields (names & types).
> One thing that is essential we get right here is that ordering of fields
should have no bearing on the generated hash. so { "foo": 7, "bar" 10 } is the
same type as { "bar": 12, "foo": 11 }, e.g. Otherwise we end up with a "type
explosion" of zillions of copies of the same thing each getting their own type
ID. If we're lucky, the server side will sort the field names when generating
the type ID on that side, and they'll all get the same ID, but then we've still
made a ton of unnecessary trips to the server.
In the tests I did, I saw that the order of the fields does not change the
result of the hash.
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:
us...@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #5249: Refactor Restore Redundancy Command
mhansonp commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r441706637
##
File path:
geode-management/src/main/java/org/apache/geode/management/operation/RestoreRedundancyRequest.java
##
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.management.operation;
+
+import java.util.List;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.api.ClusterManagementOperation;
+import org.apache.geode.management.runtime.RestoreRedundancyResults;
+
+/**
+ * Defines a distributed system request to optimize bucket allocation across
members.
+ */
+@Experimental
+public class RestoreRedundancyRequest
+implements ClusterManagementOperation {
+
+ /**
+ * see {@link #getEndpoint()}
+ */
+ public static final String RESTORE_REDUNDANCY_REBALANCE_ENDPOINT =
+ "/operations/restoreRedundancy";
+ // null means all regions included
+ private List includeRegions;
+ // null means don't exclude any regions
+ private List excludeRegions;
+ private boolean reassignPrimaries = true;
+ private String operator;
+
+ /**
+ * by default, requests all partitioned regions to be rebalanced
+ */
+ public RestoreRedundancyRequest() {}
+
+ /**
+ * copy constructor
+ */
+ public RestoreRedundancyRequest(
+ RestoreRedundancyRequest other) {
+this.setExcludeRegions(other.getExcludeRegions());
+this.setIncludeRegions(other.getIncludeRegions());
+this.setReassignPrimaries(other.getReassignPrimaries());
+this.operator = other.getOperator();
+ }
+
+ /***
+ * Returns the list of regions to be rebalanced (or an empty list for
all-except-excluded)
+ */
+ public List getIncludeRegions() {
+return includeRegions;
+ }
+
+ /**
+ * requests rebalance of the specified region(s) only. When at least one
region is specified, this
+ * takes precedence over any excluded regions.
+ */
+ public void setIncludeRegions(List includeRegions) {
+this.includeRegions = includeRegions;
+ }
+
+ /***
+ * Returns the list of regions NOT to be rebalanced (iff {@link
#getIncludeRegions()} is empty)
+ */
+ public List getExcludeRegions() {
+return excludeRegions;
+ }
+
+ /**
+ * excludes specific regions from the rebalance, if {@link
#getIncludeRegions()} is empty,
+ * otherwise has no effect
+ * default: no regions are excluded
+ */
+ public void setExcludeRegions(List excludeRegions) {
+this.excludeRegions = excludeRegions;
+ }
+
+ public void setReassignPrimaries(boolean reassignPrimaries) {
+this.reassignPrimaries = reassignPrimaries;
+ }
+
+ public boolean getReassignPrimaries() {
+return reassignPrimaries;
+ }
+
+ @Override
+ @JsonIgnore
+ public String getEndpoint() {
+return RESTORE_REDUNDANCY_REBALANCE_ENDPOINT;
+ }
+
+ @Override
+ public String getOperator() {
+return operator;
+ }
+
+ public void setOperator(String operator) {
Review comment:
I believe this is used by serialization.
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:
us...@infra.apache.org
[GitHub] [geode] bschuchardt commented on a change in pull request #5253: GEODE-8259: DSFIDSerializerImpl should handle RTE the same as Exception
bschuchardt commented on a change in pull request #5253:
URL: https://github.com/apache/geode/pull/5253#discussion_r441676962
##
File path:
geode-core/src/main/java/org/apache/geode/cache/client/internal/OpExecutorImpl.java
##
@@ -556,8 +556,8 @@ protected void handleException(Throwable e, Connection
conn, int retryCount, boo
title = null;
exToThrow = new ServerOperationException(e);
} else if (e instanceof SerializationException) {
- title = null; // no message
- exToThrow = new ServerOperationException(e);
+ title = "Unexpected SerializationException";
+ // exToThrow = new ServerOperationException(e);
Review comment:
you should remove this commented-out code
##
File path:
geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java
##
@@ -2506,153 +2506,156 @@ public static Object basicReadObject(final DataInput
in)
throw new IOException("Unknown header byte: " + header);
}
-switch (headerDSCode) {
- case DS_FIXED_ID_BYTE:
-return dsfidFactory.create(in.readByte(), in);
- case DS_FIXED_ID_SHORT:
-return dsfidFactory.create(in.readShort(), in);
- case DS_FIXED_ID_INT:
-return dsfidFactory.create(in.readInt(), in);
- case DS_NO_FIXED_ID:
- case DATA_SERIALIZABLE:
-return readDataSerializable(in);
- case NULL:
- case NULL_STRING:
-return null;
- case STRING:
-return readStringUTFFromDataInput(in);
- case HUGE_STRING:
-return readHugeStringFromDataInput(in);
- case STRING_BYTES:
-return readStringBytesFromDataInput(in, in.readUnsignedShort());
- case HUGE_STRING_BYTES:
-return readStringBytesFromDataInput(in, in.readInt());
- case CLASS:
-return readClass(in);
- case DATE:
-return readDate(in);
- case FILE:
-return readFile(in);
- case INET_ADDRESS:
-return readInetAddress(in);
- case BOOLEAN:
-return readBoolean(in);
- case CHARACTER:
-return readCharacter(in);
- case BYTE:
-return readByte(in);
- case SHORT:
-return readShort(in);
- case INTEGER:
-return readInteger(in);
- case LONG:
-return readLong(in);
- case FLOAT:
-return readFloat(in);
- case DOUBLE:
-return readDouble(in);
- case BYTE_ARRAY:
-return readByteArray(in);
- case ARRAY_OF_BYTE_ARRAYS:
-return readArrayOfByteArrays(in);
- case SHORT_ARRAY:
-return readShortArray(in);
- case STRING_ARRAY:
-return readStringArray(in);
- case INT_ARRAY:
-return readIntArray(in);
- case LONG_ARRAY:
-return readLongArray(in);
- case FLOAT_ARRAY:
-return readFloatArray(in);
- case DOUBLE_ARRAY:
-return readDoubleArray(in);
- case BOOLEAN_ARRAY:
-return readBooleanArray(in);
- case CHAR_ARRAY:
-return readCharArray(in);
- case OBJECT_ARRAY:
-return readObjectArray(in);
- case ARRAY_LIST:
-return readArrayList(in);
- case LINKED_LIST:
-return readLinkedList(in);
- case HASH_SET:
-return readHashSet(in);
- case LINKED_HASH_SET:
-return readLinkedHashSet(in);
- case HASH_MAP:
-return readHashMap(in);
- case IDENTITY_HASH_MAP:
-return readIdentityHashMap(in);
- case HASH_TABLE:
-return readHashtable(in);
- case CONCURRENT_HASH_MAP:
-return readConcurrentHashMap(in);
- case PROPERTIES:
-return readProperties(in);
- case TIME_UNIT:
-return readTimeUnit(in);
- case USER_CLASS:
-return readUserObject(in, in.readByte());
- case USER_CLASS_2:
-return readUserObject(in, in.readShort());
- case USER_CLASS_4:
-return readUserObject(in, in.readInt());
- case VECTOR:
-return readVector(in);
- case STACK:
-return readStack(in);
- case TREE_MAP:
-return readTreeMap(in);
- case TREE_SET:
-return readTreeSet(in);
- case BOOLEAN_TYPE:
-return Boolean.TYPE;
- case CHARACTER_TYPE:
-return Character.TYPE;
- case BYTE_TYPE:
-return Byte.TYPE;
- case SHORT_TYPE:
-return Short.TYPE;
- case INTEGER_TYPE:
-return Integer.TYPE;
- case LONG_TYPE:
-return Long.TYPE;
- case FLOAT_TYPE:
-return Float.TYPE;
- case DOUBLE_TYPE:
-return Double.TYPE;
- case VOID_TYPE:
-return Void.TYPE;
- case USER_DATA_SERIALIZABLE:
-return readUserDataSerializable(in, in.readByte());
- case USER_DATA_SERIALIZABLE_2:
-return readUserDataSerializable(in, in.readShort());
- case USER_DATA_SERIALIZABLE_4:
-return readUserDataSerializable(in, in.readInt());
- case SERIALIZABLE:
-return
[GitHub] [geode] yozaner1324 opened a new pull request #5264: GEODE-8271 - Update Spring artifacts to latest patch versions.
yozaner1324 opened a new pull request #5264: URL: https://github.com/apache/geode/pull/5264 Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, check Concourse for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. 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: us...@infra.apache.org
[GitHub] [geode] sabbeyPivotal opened a new pull request #5263: GEODE-8270: Reorganize Redis API for Geode test packages to match main package structure
sabbeyPivotal opened a new pull request #5263: URL: https://github.com/apache/geode/pull/5263 Reorganize test packages to match main package structure so it's easier to find relevant tests 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: us...@infra.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #5257: GEODE-8251: make sure Configuration can be deserialized post 1.12.
jinmeiliao commented on pull request #5257: URL: https://github.com/apache/geode/pull/5257#issuecomment-645455002 > > > What's the point for these changes? > > > I have ran the test case without them and it passes. > > > Also, I have searched in the Geode code for the use of this file and have only found it in `ObjectInputStreamFilterWrapper.java`. From what I see there, only the first part of every line, the class name, is used. > > > What am I missing? > > > > > > the previous version of Deployment has variable name "jarFileName" instead of "fileName", when we are deserializing a previous version bytes, if the variable name doesn't match, it won't take the value. We should have a test to verify that, but I saw the behavior in the debugger. > > I understand that. What I do not get are the changes in the > > > > What's the point for these changes? > > > I have ran the test case without them and it passes. > > > Also, I have searched in the Geode code for the use of this file and have only found it in `ObjectInputStreamFilterWrapper.java`. From what I see there, only the first part of every line, the class name, is used. > > > What am I missing? > > > > > > the previous version of Deployment has variable name "jarFileName" instead of "fileName", when we are deserializing a previous version bytes, if the variable name doesn't match, it won't take the value. We should have a test to verify that, but I saw the behavior in the debugger. > > I understand the change in the members of the class. What I do not get is why it needs to be reflected in sanctioned-geode-management-serializables.txt. > Who is accessing that file? the `AnalyzeManagementSerializablesJUnitTest` is making sure that we don't make random changes in those serialized classes. Every change in the sanction list poses a potential upgrade issue. So the fact that the test is broken will make the developer think twice about the changes he/she is making to the class. 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: us...@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5262: GEODE-8269: Improve test coverage
sabbeyPivotal commented on a change in pull request #5262:
URL: https://github.com/apache/geode/pull/5262#discussion_r441615964
##
File path:
geode-redis/src/test/java/org/apache/geode/redis/internal/executor/key/ExpireAtExecutorJUnitTest.java
##
@@ -34,30 +37,30 @@
@Test
public void calledWithTooFewCommandArguments_returnsError() {
-List commandsAsBytesWithTooFewArguments = new ArrayList<>();
-commandsAsBytesWithTooFewArguments.add("EXPIREAT".getBytes());
-commandsAsBytesWithTooFewArguments.add("key".getBytes());
-Command command = new Command(commandsAsBytesWithTooFewArguments);
+List commandAsBytes = new ArrayList<>();
+commandAsBytes.add("EXPIREAT".getBytes());
+commandAsBytes.add("key".getBytes());
+Command command = new Command(commandAsBytes);
-RedisResponse response =
-new ExpireAtExecutor().executeCommand(command, mockContext());
+Throwable thrown = catchThrowable(() -> command.execute(mockContext()));
-assertThat(response.toString()).startsWith("-ERR The wrong number of
arguments");
+AssertionsForClassTypes.assertThat(thrown).hasMessageContaining("wrong
number of arguments");
+
AssertionsForClassTypes.assertThat(thrown).isInstanceOf(RedisParametersMismatchException.class);
}
@Test
public void calledWithTooManyCommandArguments_returnsError() {
-List commandsAsBytesWithTooFewArguments = new ArrayList<>();
-commandsAsBytesWithTooFewArguments.add("EXPIREAT".getBytes());
-commandsAsBytesWithTooFewArguments.add("key".getBytes());
-commandsAsBytesWithTooFewArguments.add("1".getBytes());
-commandsAsBytesWithTooFewArguments.add("extra-argument".getBytes());
-Command command = new Command(commandsAsBytesWithTooFewArguments);
+List commandsAsBytes = new ArrayList<>();
+commandsAsBytes.add("EXPIREAT".getBytes());
+commandsAsBytes.add("key".getBytes());
+commandsAsBytes.add("1".getBytes());
+commandsAsBytes.add("extra-argument".getBytes());
+Command command = new Command(commandsAsBytes);
-RedisResponse response =
-new ExpireAtExecutor().executeCommand(command, mockContext());
+Throwable thrown = catchThrowable(() -> command.execute(mockContext()));
-assertThat(response.toString()).startsWith("-ERR The wrong number of
arguments");
+AssertionsForClassTypes.assertThat(thrown).hasMessageContaining("wrong
number of arguments");
+
AssertionsForClassTypes.assertThat(thrown).isInstanceOf(RedisParametersMismatchException.class);
Review comment:
Definitely. Editing those now.
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:
us...@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5262: GEODE-8269: Improve test coverage
sabbeyPivotal commented on a change in pull request #5262:
URL: https://github.com/apache/geode/pull/5262#discussion_r441615674
##
File path:
geode-redis/src/test/java/org/apache/geode/redis/internal/executor/key/ExpireAtExecutorJUnitTest.java
##
@@ -34,30 +37,30 @@
@Test
public void calledWithTooFewCommandArguments_returnsError() {
-List commandsAsBytesWithTooFewArguments = new ArrayList<>();
-commandsAsBytesWithTooFewArguments.add("EXPIREAT".getBytes());
-commandsAsBytesWithTooFewArguments.add("key".getBytes());
-Command command = new Command(commandsAsBytesWithTooFewArguments);
+List commandAsBytes = new ArrayList<>();
+commandAsBytes.add("EXPIREAT".getBytes());
+commandAsBytes.add("key".getBytes());
+Command command = new Command(commandAsBytes);
-RedisResponse response =
-new ExpireAtExecutor().executeCommand(command, mockContext());
+Throwable thrown = catchThrowable(() -> command.execute(mockContext()));
Review comment:
yeah! good catch.
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:
us...@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #5257: GEODE-8251: make sure Configuration can be deserialized post 1.12.
albertogpz commented on pull request #5257: URL: https://github.com/apache/geode/pull/5257#issuecomment-645426599 > > What's the point for these changes? > > I have ran the test case without them and it passes. > > Also, I have searched in the Geode code for the use of this file and have only found it in `ObjectInputStreamFilterWrapper.java`. From what I see there, only the first part of every line, the class name, is used. > > What am I missing? > > the previous version of Deployment has variable name "jarFileName" instead of "fileName", when we are deserializing a previous version bytes, if the variable name doesn't match, it won't take the value. We should have a test to verify that, but I saw the behavior in the debugger. I understand that. What I do not get are the changes in the > > What's the point for these changes? > > I have ran the test case without them and it passes. > > Also, I have searched in the Geode code for the use of this file and have only found it in `ObjectInputStreamFilterWrapper.java`. From what I see there, only the first part of every line, the class name, is used. > > What am I missing? > > the previous version of Deployment has variable name "jarFileName" instead of "fileName", when we are deserializing a previous version bytes, if the variable name doesn't match, it won't take the value. We should have a test to verify that, but I saw the behavior in the debugger. I understand the change in the members of the class. What I do not get is why it needs to be reflected in sanctioned-geode-management-serializables.txt. Who is accessing that file? 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: us...@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey merged pull request #620: GEODE-8262: Replace insensitive terms in dunit framework
pdxcodemonkey merged pull request #620: URL: https://github.com/apache/geode-native/pull/620 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: us...@infra.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #5257: GEODE-8251: make sure Configuration can be deserialized post 1.12.
jinmeiliao commented on pull request #5257: URL: https://github.com/apache/geode/pull/5257#issuecomment-645409764 @albertogpz the pipeline tests failed because we added GEODE_HOME environment variable in all tests environment, some tests are able to find the war files thus starting the web services, thus getting the address bind exception. Is there any way we can add that environment variable only to one test? 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: us...@infra.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #5257: GEODE-8251: make sure Configuration can be deserialized post 1.12.
jinmeiliao commented on pull request #5257: URL: https://github.com/apache/geode/pull/5257#issuecomment-645408502 > What's the point for these changes? > I have ran the test case without them and it passes. > Also, I have searched in the Geode code for the use of this file and have only found it in `ObjectInputStreamFilterWrapper.java`. From what I see there, only the first part of every line, the class name, is used. > What am I missing? the previous version of Deployment has variable name "jarFileName" instead of "fileName", when we are deserializing a previous version bytes, if the variable name doesn't match, it won't take the value. We should have a test to verify that, but I saw the behavior in the debugger. 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: us...@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5257: GEODE-8251: make sure Configuration can be deserialized post 1.12.
jinmeiliao commented on a change in pull request #5257:
URL: https://github.com/apache/geode/pull/5257#discussion_r441586414
##
File path:
geode-core/src/upgradeTest/java/org/apache/geode/internal/cache/rollingupgrade/RollingUpgradeWithGfshDUnitTest.java
##
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.rollingupgrade;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Collection;
+import java.util.List;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import org.apache.geode.internal.UniquePortSupplier;
+import org.apache.geode.test.compiler.ClassBuilder;
+import org.apache.geode.test.junit.categories.BackwardCompatibilityTest;
+import org.apache.geode.test.junit.rules.gfsh.GfshExecution;
+import org.apache.geode.test.junit.rules.gfsh.GfshRule;
+import org.apache.geode.test.junit.rules.gfsh.GfshScript;
+import
org.apache.geode.test.junit.runners.CategoryWithParameterizedRunnerFactory;
+import org.apache.geode.test.version.TestVersion;
+import org.apache.geode.test.version.VersionManager;
+
+/**
+ * This test iterates through the versions of Geode and executes client
compatibility with
+ * the current version of Geode.
+ */
+@Category({BackwardCompatibilityTest.class})
+@RunWith(Parameterized.class)
+@Parameterized.UseParametersRunnerFactory(CategoryWithParameterizedRunnerFactory.class)
+public class RollingUpgradeWithGfshDUnitTest {
+ private final UniquePortSupplier portSupplier = new UniquePortSupplier();
+ private final String oldVersion;
+
+ @Parameterized.Parameters(name = "{0}")
+ public static Collection data() {
+List result =
VersionManager.getInstance().getVersionsWithoutCurrent();
+result.removeIf(s -> TestVersion.compare(s, "1.10.0") < 0);
Review comment:
There is no change in the the Configuration object except in 1.12, so
only to test a few version before it and after it. Start from 1.10 just to save
a few test cycles, but we can test the previous ones.
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:
us...@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5262: GEODE-8269: Improve test coverage
jdeppe-pivotal commented on a change in pull request #5262:
URL: https://github.com/apache/geode/pull/5262#discussion_r441579410
##
File path:
geode-redis/src/test/java/org/apache/geode/redis/internal/executor/key/ExpireAtExecutorJUnitTest.java
##
@@ -34,30 +37,30 @@
@Test
public void calledWithTooFewCommandArguments_returnsError() {
-List commandsAsBytesWithTooFewArguments = new ArrayList<>();
-commandsAsBytesWithTooFewArguments.add("EXPIREAT".getBytes());
-commandsAsBytesWithTooFewArguments.add("key".getBytes());
-Command command = new Command(commandsAsBytesWithTooFewArguments);
+List commandAsBytes = new ArrayList<>();
+commandAsBytes.add("EXPIREAT".getBytes());
+commandAsBytes.add("key".getBytes());
+Command command = new Command(commandAsBytes);
-RedisResponse response =
-new ExpireAtExecutor().executeCommand(command, mockContext());
+Throwable thrown = catchThrowable(() -> command.execute(mockContext()));
-assertThat(response.toString()).startsWith("-ERR The wrong number of
arguments");
+AssertionsForClassTypes.assertThat(thrown).hasMessageContaining("wrong
number of arguments");
+
AssertionsForClassTypes.assertThat(thrown).isInstanceOf(RedisParametersMismatchException.class);
}
@Test
public void calledWithTooManyCommandArguments_returnsError() {
-List commandsAsBytesWithTooFewArguments = new ArrayList<>();
-commandsAsBytesWithTooFewArguments.add("EXPIREAT".getBytes());
-commandsAsBytesWithTooFewArguments.add("key".getBytes());
-commandsAsBytesWithTooFewArguments.add("1".getBytes());
-commandsAsBytesWithTooFewArguments.add("extra-argument".getBytes());
-Command command = new Command(commandsAsBytesWithTooFewArguments);
+List commandsAsBytes = new ArrayList<>();
+commandsAsBytes.add("EXPIREAT".getBytes());
+commandsAsBytes.add("key".getBytes());
+commandsAsBytes.add("1".getBytes());
+commandsAsBytes.add("extra-argument".getBytes());
+Command command = new Command(commandsAsBytes);
-RedisResponse response =
-new ExpireAtExecutor().executeCommand(command, mockContext());
+Throwable thrown = catchThrowable(() -> command.execute(mockContext()));
-assertThat(response.toString()).startsWith("-ERR The wrong number of
arguments");
+AssertionsForClassTypes.assertThat(thrown).hasMessageContaining("wrong
number of arguments");
+
AssertionsForClassTypes.assertThat(thrown).isInstanceOf(RedisParametersMismatchException.class);
Review comment:
Maybe the same thing, but at least for consistency, `assertThat` should
come from the `org.assertj.core.api.Assertions` package.
##
File path:
geode-redis/src/test/java/org/apache/geode/redis/internal/executor/key/KeysExecutorJUnitTest.java
##
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ *
+ */
+
+package org.apache.geode.redis.internal.executor.key;
+
+import static org.assertj.core.api.AssertionsForClassTypes.catchThrowable;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import io.netty.buffer.UnpooledByteBufAllocator;
+import org.assertj.core.api.AssertionsForClassTypes;
+import org.junit.Test;
+
+import
org.apache.geode.redis.internal.ParameterRequirements.RedisParametersMismatchException;
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+
+public class KeysExecutorJUnitTest {
+
+ @Test
+ public void calledWithTooFewOptions_returnsError() {
+List commandsAsBytes = new ArrayList<>();
+commandsAsBytes.add("KEYS".getBytes());
+Command command = new Command(commandsAsBytes);
+
+Throwable thrown = catchThrowable(() -> command.execute(mockContext()));
+
+AssertionsForClassTypes.assertThat(thrown).hasMessageContaining("wrong
number of arguments");
+
AssertionsForClassTypes.assertThat(thrown).isInstanceOf(RedisParametersMismatchException.class);
+ }
+
+ @Test
+ public void calledWithTooManyOptions_returnsError() {
+List commandsAsBytes = new ArrayList<>();
+
[GitHub] [geode] sabbeyPivotal opened a new pull request #5262: GEODE-8269: Improve test coverage
sabbeyPivotal opened a new pull request #5262: URL: https://github.com/apache/geode/pull/5262 Analyzed test coverage via Intellij and by looking through all tests. There were some test cases missing. We either added them or created additional stories in Tracker. 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: us...@infra.apache.org
[GitHub] [geode] albertogpz commented on a change in pull request #5257: GEODE-8251: make sure Configuration can be deserialized post 1.12.
albertogpz commented on a change in pull request #5257:
URL: https://github.com/apache/geode/pull/5257#discussion_r441382563
##
File path:
geode-core/src/upgradeTest/java/org/apache/geode/internal/cache/rollingupgrade/RollingUpgradeWithGfshDUnitTest.java
##
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.rollingupgrade;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Collection;
+import java.util.List;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import org.apache.geode.internal.UniquePortSupplier;
+import org.apache.geode.test.compiler.ClassBuilder;
+import org.apache.geode.test.junit.categories.BackwardCompatibilityTest;
+import org.apache.geode.test.junit.rules.gfsh.GfshExecution;
+import org.apache.geode.test.junit.rules.gfsh.GfshRule;
+import org.apache.geode.test.junit.rules.gfsh.GfshScript;
+import
org.apache.geode.test.junit.runners.CategoryWithParameterizedRunnerFactory;
+import org.apache.geode.test.version.TestVersion;
+import org.apache.geode.test.version.VersionManager;
+
+/**
+ * This test iterates through the versions of Geode and executes client
compatibility with
+ * the current version of Geode.
+ */
+@Category({BackwardCompatibilityTest.class})
+@RunWith(Parameterized.class)
+@Parameterized.UseParametersRunnerFactory(CategoryWithParameterizedRunnerFactory.class)
+public class RollingUpgradeWithGfshDUnitTest {
+ private final UniquePortSupplier portSupplier = new UniquePortSupplier();
+ private final String oldVersion;
+
+ @Parameterized.Parameters(name = "{0}")
+ public static Collection data() {
+List result =
VersionManager.getInstance().getVersionsWithoutCurrent();
+result.removeIf(s -> TestVersion.compare(s, "1.10.0") < 0);
Review comment:
Why start from this version? Is the upgrade from previous versions not
supported?
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:
us...@infra.apache.org
[GitHub] [geode] mivanac merged pull request #5182: GEODE-7591: Fix for hang in ClusterDistributionManager
mivanac merged pull request #5182: URL: https://github.com/apache/geode/pull/5182 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: us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal merged pull request #5237: GEODE-8268: clean up ExecutionHandlerContext
dschneider-pivotal merged pull request #5237: URL: https://github.com/apache/geode/pull/5237 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: us...@infra.apache.org
