[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #618: GEODE-8246: Enfore No Shadow as error
pivotal-jbarrett commented on a change in pull request #618:
URL: https://github.com/apache/geode-native/pull/618#discussion_r439207032
##
File path: cppcache/src/MapSegment.cpp
##
@@ -731,7 +731,7 @@ GfErrType
MapSegment::isTombstone(std::shared_ptr key,
bool& result) {
std::shared_ptr value;
std::shared_ptr mePtr;
- const auto& find = m_map->find(key);
+ auto find = m_map->find(key);
Review comment:
Rather than incurring the copy cost here just scope this variable with
some bracing. Or use different variable names for each `find` result. If they
are immutable the compiler has a better chance optimizing them away.
##
File path: cppcache/src/LRUList.cpp
##
@@ -102,27 +102,25 @@ void LRUList::getLRUEntry(
template
typename LRUList::LRUListNode*
LRUList::getHeadNode(bool& isLast) {
- std::lock_guard lk(m_headLock);
+ std::lock_guard headSpinlock(m_headLock);
LRUListNode* result = m_headNode;
LRUListNode* nextNode;
- {
-std::lock_guard lk(m_tailLock);
-
-nextNode = m_headNode->getNextLRUListNode();
-if (nextNode == nullptr) {
- // last one in the list...
- isLast = true;
- std::shared_ptr entry;
- result->getEntry(entry);
- if (entry->getLRUProperties().testEvicted()) {
-// list is empty.
-return nullptr;
- } else {
-entry->getLRUProperties().setEvicted();
-return result;
- }
+ std::lock_guard tailSpinlock(m_tailLock);
Review comment:
The scope of the `m_tailLock` has been extended to the of of this method
along with the `m_headLock`.
##
File path: cppcache/src/ClientMetadataService.cpp
##
@@ -765,18 +766,17 @@ void
ClientMetadataService::markPrimaryBucketForTimeoutButLookSecondaryBucket(
serverLocation, version);
std::shared_ptr cptr = nullptr;
- {
-boost::shared_lock lock(
-m_regionMetadataLock);
-const auto& cptrIter = m_regionMetaDataMap.find(region->getFullPath());
-if (cptrIter != m_regionMetaDataMap.end()) {
- cptr = cptrIter->second;
-}
+ boost::shared_lock boostRegionMetadataLock(
Review comment:
You have changed the scope of this lock by removing the block
surrounding it. The lock is now held until the end of this method rather than
being released after line 780.
##
File path: cppcache/src/TcrEndpoint.cpp
##
@@ -439,7 +440,7 @@ GfErrType TcrEndpoint::registerDM(bool clientNotification,
bool isSecondary,
"channel for endpoint %s",
m_name.c_str());
// setup notification channel for the first region
- std::lock_guard guard(
+ std::lock_guard notifyReceiverLockGuard(
Review comment:
Oh wow, that was a bad one. Confusing to rationalize when `guard` would
be released.
##
File path: cppcache/src/Log.cpp
##
@@ -257,7 +257,7 @@ void Log::init(LogLevel level, const char* logFileName,
int32_t logFileLimit,
if (status != -1) {
for (int index = 0; index < sds.length(); ++index) {
std::string strname = ACE::basename(sds[index]->d_name);
-size_t fileExtPos = strname.find_last_of('.', strname.length());
+fileExtPos = strname.find_last_of('.', strname.length());
Review comment:
Change previous declaration to `auto`.
##
File path: cppcache/src/Log.cpp
##
@@ -280,7 +280,7 @@ void Log::init(LogLevel level, const char* logFileName,
int32_t logFileLimit,
std::string extName;
std::string newfilestr;
- int32_t len = static_cast(g_logFileWithExt->length());
+ len = static_cast(g_logFileWithExt->length());
Review comment:
Change previous declaration to `auto`.
##
File path: cppcache/src/Log.cpp
##
@@ -657,7 +657,7 @@ void Log::put(LogLevel level, const char* msg) {
char printmsg[256];
std::snprintf(printmsg, 256, "%s\t%s\n", "Could not delete",
fileInfo[fileIndex].first.c_str());
- int numChars =
+ numChars =
Review comment:
Change previous declaration to `auto`.
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] moleske opened a new pull request #618: GEODE-8246: Enfore No Shadow as error
moleske opened a new pull request #618: URL: https://github.com/apache/geode-native/pull/618 There's a lot of files that were touched. If folks have ideas on how to break it up such that it is easier to review let me know. I basically took the approach of remove it from CMakeLists.txt, then go fix all the errors (with occasional aggressive rename). Most of the no shadow errors were from the older testing suite. Tests run and passing on Kubuntu 20.04, Mac Catalina, and Windows 10. 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 #5184: change more string commands to have CommandFunction support
lgtm-com[bot] commented on pull request #5184: URL: https://github.com/apache/geode/pull/5184#issuecomment-643018762 This pull request **fixes 2 alerts** when merging 5d237f81857b60b8ccf1bd8611c5ce3c984c8c3c into cb5990cd437244bc0ac8abdc6e12552e686e7c7a - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-06b39755eacf4cba083b2cc3504ccd1c1c0c63ba) **fixed alerts:** * 1 for Useless comparison test * 1 for Array index out of bounds 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 #5184: change more string commands to have CommandFunction support
dschneider-pivotal commented on pull request #5184: URL: https://github.com/apache/geode/pull/5184#issuecomment-642992518 I fixed the issues Jens found and added integration tests for all these string commands 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 merged pull request #5244: GEODE-8221: Refactor tests to run in appropriate projects.
pivotal-jbarrett merged pull request #5244: URL: https://github.com/apache/geode/pull/5244 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] karensmolermiller commented on a change in pull request #617: Update C++ serialization example
karensmolermiller commented on a change in pull request #617:
URL: https://github.com/apache/geode-native/pull/617#discussion_r439120598
##
File path:
docs/geode-native-docs-cpp/serialization/cpp-serialization/pdxserializable-interface.html.md.erb
##
@@ -27,88 +27,60 @@ A domain class should serialize and de-serialize all its
member fields in the sa
Use this procedure to program your domain object for PDX serialization using
the `PdxSerializable` abstract class.
-1. In your domain class, implement `PdxSerializable`. Example:
+1. In your domain class, implement `PdxSerializable`. For example:
``` pre
-class PdxObject: public PdxSerializable
+class Order : public PdxSerializable {
```
-2. Program the `toData` function to serialize your object as required by your
application.
-
+2. Program the `toData` method to serialize your object as required by your
application. (See `markIdentityField` in a later step for an optimization that
you can apply to this code sample.)
+
+``` cpp
+void Order::toData(PdxWriter& pdxWriter) const {
+ pdxWriter.writeInt(ORDER_ID_KEY_, order_id_);
+ pdxWriter.writeString(NAME_KEY_, name_);
+ pdxWriter.writeShort(QUANTITY_KEY_, quantity_);
+}
+```
+
If you also use PDX serialization in Java or .NET for the object,
serialize the object in the same way for each language. Serialize the same
fields in the same order and mark the same identity fields.
3. Program the `fromData` method to read your data fields from the serialized
form into the object's fields.
-
-In your `fromData` implementation, use the same name as you did in
`toData` and call the read operations in the same order as you called the write
operations in your `toData` implementation.
-4. Optionally, program your domain object's `hashCode` and equality methods.
-
-Use the `markIdentityField` method to indicate that the given field name
should be included in `hashCode` and equality checks of this object on a server.
-
-The fields that are marked as identity fields are used to generate the
`hashCode` and equality methods of PdxInstance. Because of this, the identity
fields should themselves either be primitives, or implement `hashCode` and
equals.
-
-If no fields are set as identity fields, then all fields will be used in
`hashCode` and equality checks. The identity fields should make marked after
they are written using a `write*` method.
-
-For example:
-
-``` cpp
-class PdxObject: public PdxSerializable {
+```cpp
+void Order::fromData(PdxReader& pdxReader) {
+ order_id_ = pdxReader.readInt(ORDER_ID_KEY_);
+ name_ = pdxReader.readString(NAME_KEY_);
+ quantity_ = pdxReader.readShort(QUANTITY_KEY_);
+}
+```
-private:
-uint32_t m_id;
-char* m_str;
+In your `fromData` implementation, use the same name as you did in
`toData` and call the read operations in the same order as you called the write
operations in your `toData` implementation.
-public:
-PdxObject(){};
-PdxObject(uint32_t id, char* str);
-virtual ~PdxObject();
+4. Optionally, program your domain object's `hashCode` and equality methods.
When you do so, you can optimize those methods by specifying the _identity
fields_
+to be used in comparisons.
+
+If no fields are set as identity fields, then all fields will be used in
`hashCode` and equality checks, so marking identity fields improves the
efficiency
+of hashing and equality operations.
+
+Use the `markIdentityField` method to indicate that the given field name
should be included in `hashCode` and equality checks of this object on a server.
+It is important that the fields used by your equality method and
`hashCode` implementations are the same fields that you mark as identity fields.
+
+Expanding the code sample from the description of the `toData` method,
above:
-uint32_t getID() {
-return m_id;
-}
+``` cpp
+void Order::toData(PdxWriter& pdxWriter) const {
+ pdxWriter.writeInt(ORDER_ID_KEY_, order_id_);
+ pdxWriter.markIdentityField(ORDER_ID_KEY_);
-char* getStr(){
-return m_str;
-}
+ pdxWriter.writeString(NAME_KEY_, name_);
+ pdxWriter.markIdentityField(NAME_KEY_);
-virtual void toData(PdxWriterPtr pw) const;
-virtual void fromData(PdxReaderPtr pr);
-CacheableStringPtr toString() const;
-virtual char* getClassName() const;
-static Cacheable* createDeserializable() {
-return new PdxObject();
+ pdxWriter.writeShort(QUANTITY_KEY_, quantity_);
+ pdxWriter.markIdentityField(QUANTITY_KEY_);
}
-};
-
-PdxObject::PdxObject(uint32_t i, char* str) {
-m_id = i;
-m_str = str;
-}
-
-PdxObject::~PdxObject() {
-}
-
-void PdxObject::toData( PdxWriterPtr pw ) const {
-pw->writeInt("id", m_id);
- pw->markIdentityField("id");
-pw->writeString("str",
[GitHub] [geode] jchen21 opened a new pull request #5247: GEODE-7681 Gather partitioned region clear operation performance metrics
jchen21 opened a new pull request #5247: URL: https://github.com/apache/geode/pull/5247 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] jinmeiliao commented on a change in pull request #5187: GEODE-8179: gfsh query cmd returns incorrect results if '=' sign is missing
jinmeiliao commented on a change in pull request #5187:
URL: https://github.com/apache/geode/pull/5187#discussion_r439097235
##
File path:
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
##
@@ -123,18 +123,25 @@ else if (insideQuoteOf == c) {
List furtherSplitWithEquals = new ArrayList<>();
for (String token : splitWithWhiteSpaces) {
- // if this token has equal sign, split around the first occurrence of it
int indexOfFirstEqual = token.indexOf('=');
- if (indexOfFirstEqual < 0) {
+ int indexOfFirstDoubleQuote = token.indexOf('"');
+ int indexOfFirstSingleQuote = token.indexOf('\'');
+ if (indexOfFirstEqual < 0 || token.startsWith("-D")) {
furtherSplitWithEquals.add(token);
} else {
-String left = token.substring(0, indexOfFirstEqual);
-String right = token.substring(indexOfFirstEqual + 1);
-if (left.length() > 0) {
- furtherSplitWithEquals.add(left);
-}
-if (right.length() > 0) {
- furtherSplitWithEquals.add(right);
+if (indexOfFirstDoubleQuote == 0 || indexOfFirstSingleQuote == 0) {
+ furtherSplitWithEquals.add(token);
+} else if ((indexOfFirstDoubleQuote < 0 && indexOfFirstSingleQuote < 0)
+|| (indexOfFirstEqual < indexOfFirstDoubleQuote)
+|| (indexOfFirstEqual < indexOfFirstSingleQuote)) {
+ String left = token.substring(0, indexOfFirstEqual);
+ String right = token.substring(indexOfFirstEqual + 1);
+ if (left.length() > 0) {
+furtherSplitWithEquals.add(left);
+ }
+ if (right.length() > 0) {
+furtherSplitWithEquals.add(right);
+ }
}
}
}
Review comment:
this entire for loop can be simplified to:
```
for (String token : splitWithWhiteSpaces) {
// do not split with "=" if this part starts with quotes or is part of
-D
if(token.startsWith("'") || token.startsWith("\"") ||
token.startsWith("-D")) {
furtherSplitWithEquals.add(token);
continue;
}
// if this token has equal sign, split around the first occurrence of
it
int indexOfFirstEqual = token.indexOf('=');
if (indexOfFirstEqual < 0) {
furtherSplitWithEquals.add(token);
continue;
}
String left = token.substring(0, indexOfFirstEqual);
String right = token.substring(indexOfFirstEqual + 1);
if (left.length() > 0) {
furtherSplitWithEquals.add(left);
}
if (right.length() > 0) {
furtherSplitWithEquals.add(right);
}
}
```
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] davebarnes97 opened a new pull request #617: Update C++ serialization example
davebarnes97 opened a new pull request #617: URL: https://github.com/apache/geode-native/pull/617 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 #5188: GEODE-8099: add dlock around cms create/delete operations.
jinmeiliao commented on a change in pull request #5188:
URL: https://github.com/apache/geode/pull/5188#discussion_r439082696
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/BaseManagementService.java
##
@@ -94,6 +94,13 @@ public static void
setManagementService(InternalCacheForClientAccess cache,
}
}
+ @VisibleForTesting
+ public static void clearManagementService(InternalCacheForClientAccess
cache) {
+synchronized (instances) {
+ instances.remove(cache);
Review comment:
yeah, this is legacy code. for testing purpose, I had to add this 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] jhutchison commented on a change in pull request #5216: Refactor set executor
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r439078174
##
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
-List commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
-ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+List commandElementsBytes = command.getProcessedCommand();
+List optionalParameterBytes =
getOptionalParameters(commandElementsBytes);
+ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
-return doSet(command, context, keyToSet, valueToSet, redisStringCommands,
setOptions);
+return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List getOptionalParameters(List
commandElementsBytes) {
+return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands,
SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
-boolean result = redisStringCommands.set(key, value, setOptions);
+boolean setCompletedSuccessfully = redisStringCommands.set(key, value,
setOptions);
-if (result) {
+if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+} else {
+ return RedisResponse.nil();
}
-
-return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List
optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List commandElems) throws
IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long expiration = 0L;
-for (int i = 3; i < commandElems.size(); i++) {
- String current_arg =
Coder.bytesToString(commandElems.get(i)).toUpperCase();
- switch (current_arg) {
-case "KEEPTTL":
- keepTTL = true;
- break;
-case "EX":
- if (expiration != 0) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
-case "PX":
- if (expiration != 0) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
-case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
-case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
-default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+List optionalParametersStrings =
+optionalParameterBytes.stream()
+.map(item -> Coder.bytesToString(item).toUpperCase())
+.collect(Collectors.toList());
+
+throwExceptionIfUnknownParameter(optionalParametersStrings);
+throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+keepTTL = optionalParametersStrings.contains("KEEPTL");
+
+if (optionalParametersStrings.contains("PX")) {
+ String nextParameter =
+ getNextParameter("PX", optionalParametersStrings);
+
+ if (!isANumber(nextParameter)) {
+throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ expiration = parseExpirationTime("PX", nextParameter);
+ if (expiration <= 0) {
+throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
+ }
+
+} else if (optionalParametersStrings.contains("EX")) {
+ String nextParameter =
+ getNextParameter("EX", optionalParametersStrings);
+ if (!isANumber(nextParameter)) {
+throw new
[GitHub] [geode] kohlmu-pivotal merged pull request #5243: GEODE-8190: Clean up tests and automate Geode version.
kohlmu-pivotal merged pull request #5243: URL: https://github.com/apache/geode/pull/5243 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 #5246: GEODE-8221: Commits session data prior to sending output to browser
pivotal-jbarrett opened a new pull request #5246: URL: https://github.com/apache/geode/pull/5246 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] jhutchison commented on a change in pull request #5216: Refactor set executor
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r439070387
##
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
-List commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
-ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+List commandElementsBytes = command.getProcessedCommand();
+List optionalParameterBytes =
getOptionalParameters(commandElementsBytes);
+ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
-return doSet(command, context, keyToSet, valueToSet, redisStringCommands,
setOptions);
+return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List getOptionalParameters(List
commandElementsBytes) {
+return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands,
SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
-boolean result = redisStringCommands.set(key, value, setOptions);
+boolean setCompletedSuccessfully = redisStringCommands.set(key, value,
setOptions);
-if (result) {
+if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+} else {
+ return RedisResponse.nil();
}
-
-return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List
optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List commandElems) throws
IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long expiration = 0L;
-for (int i = 3; i < commandElems.size(); i++) {
- String current_arg =
Coder.bytesToString(commandElems.get(i)).toUpperCase();
- switch (current_arg) {
-case "KEEPTTL":
- keepTTL = true;
- break;
-case "EX":
- if (expiration != 0) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
-case "PX":
- if (expiration != 0) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
-case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
-case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
-default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+List optionalParametersStrings =
+optionalParameterBytes.stream()
+.map(item -> Coder.bytesToString(item).toUpperCase())
+.collect(Collectors.toList());
+
+throwExceptionIfUnknownParameter(optionalParametersStrings);
+throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+keepTTL = optionalParametersStrings.contains("KEEPTL");
+
+if (optionalParametersStrings.contains("PX")) {
+ String nextParameter =
+ getNextParameter("PX", optionalParametersStrings);
+
+ if (!isANumber(nextParameter)) {
+throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ expiration = parseExpirationTime("PX", nextParameter);
+ if (expiration <= 0) {
+throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
+ }
+
+} else if (optionalParametersStrings.contains("EX")) {
+ String nextParameter =
+ getNextParameter("EX", optionalParametersStrings);
+ if (!isANumber(nextParameter)) {
+throw new
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r439065784
##
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
-List commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
-ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+List commandElementsBytes = command.getProcessedCommand();
+List optionalParameterBytes =
getOptionalParameters(commandElementsBytes);
+ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
-return doSet(command, context, keyToSet, valueToSet, redisStringCommands,
setOptions);
+return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List getOptionalParameters(List
commandElementsBytes) {
+return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands,
SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
-boolean result = redisStringCommands.set(key, value, setOptions);
+boolean setCompletedSuccessfully = redisStringCommands.set(key, value,
setOptions);
-if (result) {
+if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+} else {
+ return RedisResponse.nil();
}
-
-return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List
optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List commandElems) throws
IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long expiration = 0L;
-for (int i = 3; i < commandElems.size(); i++) {
- String current_arg =
Coder.bytesToString(commandElems.get(i)).toUpperCase();
- switch (current_arg) {
-case "KEEPTTL":
- keepTTL = true;
- break;
-case "EX":
- if (expiration != 0) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
-case "PX":
- if (expiration != 0) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
-case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
-case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
-default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+List optionalParametersStrings =
+optionalParameterBytes.stream()
+.map(item -> Coder.bytesToString(item).toUpperCase())
+.collect(Collectors.toList());
+
+throwExceptionIfUnknownParameter(optionalParametersStrings);
+throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+keepTTL = optionalParametersStrings.contains("KEEPTL");
+
+if (optionalParametersStrings.contains("PX")) {
+ String nextParameter =
+ getNextParameter("PX", optionalParametersStrings);
+
+ if (!isANumber(nextParameter)) {
+throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ expiration = parseExpirationTime("PX", nextParameter);
+ if (expiration <= 0) {
+throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
+ }
+
+} else if (optionalParametersStrings.contains("EX")) {
+ String nextParameter =
+ getNextParameter("EX", optionalParametersStrings);
+ if (!isANumber(nextParameter)) {
+throw new
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r439055432
##
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
-List commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
-ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+List commandElementsBytes = command.getProcessedCommand();
+List optionalParameterBytes =
getOptionalParameters(commandElementsBytes);
+ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
-return doSet(command, context, keyToSet, valueToSet, redisStringCommands,
setOptions);
+return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List getOptionalParameters(List
commandElementsBytes) {
+return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands,
SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
-boolean result = redisStringCommands.set(key, value, setOptions);
+boolean setCompletedSuccessfully = redisStringCommands.set(key, value,
setOptions);
-if (result) {
+if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+} else {
+ return RedisResponse.nil();
}
-
-return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List
optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List commandElems) throws
IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long expiration = 0L;
-for (int i = 3; i < commandElems.size(); i++) {
- String current_arg =
Coder.bytesToString(commandElems.get(i)).toUpperCase();
- switch (current_arg) {
-case "KEEPTTL":
- keepTTL = true;
- break;
-case "EX":
- if (expiration != 0) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
-case "PX":
- if (expiration != 0) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
-case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
-case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
-default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+List optionalParametersStrings =
+optionalParameterBytes.stream()
+.map(item -> Coder.bytesToString(item).toUpperCase())
+.collect(Collectors.toList());
+
+throwExceptionIfUnknownParameter(optionalParametersStrings);
+throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+keepTTL = optionalParametersStrings.contains("KEEPTL");
+
+if (optionalParametersStrings.contains("PX")) {
+ String nextParameter =
+ getNextParameter("PX", optionalParametersStrings);
+
+ if (!isANumber(nextParameter)) {
+throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ expiration = parseExpirationTime("PX", nextParameter);
+ if (expiration <= 0) {
+throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
+ }
+
+} else if (optionalParametersStrings.contains("EX")) {
+ String nextParameter =
+ getNextParameter("EX", optionalParametersStrings);
+ if (!isANumber(nextParameter)) {
+throw new
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r439053138
##
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
-List commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
-ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+List commandElementsBytes = command.getProcessedCommand();
+List optionalParameterBytes =
getOptionalParameters(commandElementsBytes);
+ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
-return doSet(command, context, keyToSet, valueToSet, redisStringCommands,
setOptions);
+return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List getOptionalParameters(List
commandElementsBytes) {
+return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands,
SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
-boolean result = redisStringCommands.set(key, value, setOptions);
+boolean setCompletedSuccessfully = redisStringCommands.set(key, value,
setOptions);
-if (result) {
+if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+} else {
+ return RedisResponse.nil();
}
-
-return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List
optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List commandElems) throws
IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long expiration = 0L;
-for (int i = 3; i < commandElems.size(); i++) {
- String current_arg =
Coder.bytesToString(commandElems.get(i)).toUpperCase();
- switch (current_arg) {
-case "KEEPTTL":
- keepTTL = true;
- break;
-case "EX":
- if (expiration != 0) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
-case "PX":
- if (expiration != 0) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
-case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
-case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
-default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+List optionalParametersStrings =
+optionalParameterBytes.stream()
+.map(item -> Coder.bytesToString(item).toUpperCase())
+.collect(Collectors.toList());
+
+throwExceptionIfUnknownParameter(optionalParametersStrings);
+throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+keepTTL = optionalParametersStrings.contains("KEEPTL");
Review comment:
thanks- I'll look into why no test caught 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] jhutchison commented on a change in pull request #5216: Refactor set executor
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r439053138
##
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
-List commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
-ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+List commandElementsBytes = command.getProcessedCommand();
+List optionalParameterBytes =
getOptionalParameters(commandElementsBytes);
+ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
-return doSet(command, context, keyToSet, valueToSet, redisStringCommands,
setOptions);
+return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List getOptionalParameters(List
commandElementsBytes) {
+return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands,
SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
-boolean result = redisStringCommands.set(key, value, setOptions);
+boolean setCompletedSuccessfully = redisStringCommands.set(key, value,
setOptions);
-if (result) {
+if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+} else {
+ return RedisResponse.nil();
}
-
-return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List
optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List commandElems) throws
IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long expiration = 0L;
-for (int i = 3; i < commandElems.size(); i++) {
- String current_arg =
Coder.bytesToString(commandElems.get(i)).toUpperCase();
- switch (current_arg) {
-case "KEEPTTL":
- keepTTL = true;
- break;
-case "EX":
- if (expiration != 0) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
-case "PX":
- if (expiration != 0) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
-case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
-case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
-throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
-default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+List optionalParametersStrings =
+optionalParameterBytes.stream()
+.map(item -> Coder.bytesToString(item).toUpperCase())
+.collect(Collectors.toList());
+
+throwExceptionIfUnknownParameter(optionalParametersStrings);
+throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+keepTTL = optionalParametersStrings.contains("KEEPTL");
Review comment:
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] dschneider-pivotal commented on a change in pull request #5237: cleaned up ExecutionHandlerContext dependencies
dschneider-pivotal commented on a change in pull request #5237:
URL: https://github.com/apache/geode/pull/5237#discussion_r439030672
##
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
##
@@ -264,7 +238,7 @@ private void
moveSubscribeToNewEventLoopGroup(ChannelHandlerContext ctx, Command
if (command.isOfType(SUBSCRIBE)) {
CountDownLatch latch = new CountDownLatch(0);
ctx.channel().deregister().addListener((ChannelFutureListener) future ->
{
-subscriberEventLoopGroup.register(ctx.channel()).sync();
+server.getSubscriberGroup().register(ctx.channel()).sync();
Review comment:
What I would like to see here is that this code
(moveSubscribeToNewEventLoopGroup) to the SubscribeExecutor. Is that possible?
I don't really understand what this is about and maybe it is important to do it
after the executor has completed.
I'd also like to see all the netty specific code get moved out of
GeodeRedisServer into something like NettyServer in our netty package just to
have all the interactions with netty be in that one package. So then this code
would interact with the NettyServer for this registration. But then this code
would need to get access to the NettyServer (probably from the
GeodeRedisServer). If we could break this class's dependency on
GeodeRedisServer that might be a good thing but for now we need the
GeodeRedisServer anyway (I think so we can ask it to shutdown and if
unsupported commands are allowed).
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 #5245: reviewed ignored redis tests and enabled those that should now pass
dschneider-pivotal opened a new pull request #5245: URL: https://github.com/apache/geode/pull/5245 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] pivotal-jbarrett opened a new pull request #5244: GEODE-8221: Refactor tests to run in appropriate projects.
pivotal-jbarrett opened a new pull request #5244: URL: https://github.com/apache/geode/pull/5244 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] mmartell opened a new pull request #616: GEODE-8128: Add SNI tests using new test framework
mmartell opened a new pull request #616: URL: https://github.com/apache/geode-native/pull/616 This add 3 SNI tests to the new test framework. Note that the test which requires a working proxy and gateway server has been disabled for 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] jdeppe-pivotal commented on pull request #5184: change more string commands to have CommandFunction support
jdeppe-pivotal commented on pull request #5184: URL: https://github.com/apache/geode/pull/5184#issuecomment-642879687 `INCRBYFLOAT` gives the following error and the `redis-cli` exits. It works fine in native redis: ``` 127.0.0.1:6379> incrbyfloat foo 1.1 Error: Protocol error, got "4" as reply type byte ``` However, when I reconnect `foo` appears to have been incremented correctly. 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 pull request #5184: change more string commands to have CommandFunction support
jdeppe-pivotal commented on pull request #5184: URL: https://github.com/apache/geode/pull/5184#issuecomment-642875100 I was trying some string OPs and saw this with `BITOP NOT`: ``` java.lang.ClassCastException: org.apache.geode.internal.cache.LocalDataSet cannot be cast to org.apache.geode.internal.cache.InternalRegion at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.internal.cache.execute.InternalFunctionExecutionServiceImpl.isClientRegion(InternalFunctionExecutionSe rviceImpl.java:400) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.internal.cache.execute.InternalFunctionExecutionServiceImpl.onRegion(InternalFunctionExecutionServiceI mpl.java:146) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.cache.execute.FunctionService.onRegion(FunctionService.java:76) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.redis.internal.executor.CommandFunction.execute(CommandFunction.java:52) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.redis.internal.executor.string.RedisStringCommandsFunctionExecutor.get(RedisStringCommandsFunctionExec utor.java:57) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.redis.internal.data.RedisStringInRegion.bitop(RedisStringInRegion.java:177) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.redis.internal.executor.CommandFunction.lambda$compute$19(CommandFunction.java:177) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.redis.internal.executor.CommandFunction.compute(CommandFunction.java:335) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.redis.internal.executor.SingleResultRedisFunction.lambda$execute$0(SingleResultRedisFunction.java:51) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.internal.cache.PartitionedRegion.computeWithPrimaryLocked(PartitionedRegion.java:631) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.redis.internal.executor.SingleResultRedisFunction.computeWithPrimaryLocked(SingleResultRedisFunction.j ava:71) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.redis.internal.executor.SingleResultRedisFunction.execute(SingleResultRedisFunction.java:55) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.internal.cache.PartitionedRegionDataStore.executeOnDataStore(PartitionedRegionDataStore.java:2996) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.internal.cache.partitioned.PartitionedRegionFunctionStreamingMessage.operateOnPartitionedRegion(PartitionedRegionFunctionStreamingMessage.java:101) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.internal.cache.partitioned.PartitionMessage.process(PartitionMessage.java:333) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.distributed.internal.DistributionMessage.scheduleAction(DistributionMessage.java:376) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.distributed.internal.DistributionMessage$1.run(DistributionMessage.java:441) at Remote Member '192.168.1.5(server2:78554):41002' in java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) at Remote Member '192.168.1.5(server2:78554):41002' in java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.distributed.internal.ClusterOperationExecutors.runUntilShutdown(ClusterOperationExecutors.java:442) at Remote Member '192.168.1.5(server2:78554):41002' in org.apache.geode.distributed.internal.ClusterOperationExecutors.doFunctionExecutionThread(ClusterOperationExecutors.java:377) ``` 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 #5237: cleaned up ExecutionHandlerContext dependencies
jdeppe-pivotal commented on a change in pull request #5237:
URL: https://github.com/apache/geode/pull/5237#discussion_r438999736
##
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
##
@@ -264,7 +238,7 @@ private void
moveSubscribeToNewEventLoopGroup(ChannelHandlerContext ctx, Command
if (command.isOfType(SUBSCRIBE)) {
CountDownLatch latch = new CountDownLatch(0);
ctx.channel().deregister().addListener((ChannelFutureListener) future ->
{
-subscriberEventLoopGroup.register(ctx.channel()).sync();
+server.getSubscriberGroup().register(ctx.channel()).sync();
Review comment:
I wonder if there is some 'feature-envy' creep happening here. What do
you think about pushing the registration functionality onto the `server`? I'm
also fine with this approach.
##
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
##
@@ -264,7 +238,7 @@ private void
moveSubscribeToNewEventLoopGroup(ChannelHandlerContext ctx, Command
if (command.isOfType(SUBSCRIBE)) {
CountDownLatch latch = new CountDownLatch(0);
Review comment:
This looks like a bug in that the latch should be initialized with `1`.
This is my bug :(.
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 #5184: change more string commands to have CommandFunction support
lgtm-com[bot] commented on pull request #5184: URL: https://github.com/apache/geode/pull/5184#issuecomment-642860272 This pull request **fixes 2 alerts** when merging 59c4b94d56ba4ccdc1c473f57fef0cf4d0e02478 into 4a0eeeb57ccfa59ace909b55b6f228deac7621cf - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-311a77d59154a1d47650b977bf5f8475349fc72f) **fixed alerts:** * 1 for Useless comparison test * 1 for Array index out of bounds 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 merged pull request #5241: GEODE-8243: Use java.exe on Windows in Launcher tests
kirklund merged pull request #5241: URL: https://github.com/apache/geode/pull/5241 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] yozaner1324 opened a new pull request #5243: Clean up tests and automate Geode version.
yozaner1324 opened a new pull request #5243: URL: https://github.com/apache/geode/pull/5243 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 commented on a change in pull request #5240: GEODE-8242: Add HSTRLEN redis command
dschneider-pivotal commented on a change in pull request #5240:
URL: https://github.com/apache/geode/pull/5240#discussion_r438972373
##
File path:
geode-redis/src/integrationTest/java/org/apache/geode/redis/HashesIntegrationTest.java
##
@@ -150,6 +150,15 @@ public void testHDelDeletesKeyWhenHashIsEmpty() {
assertThat(jedis.exists("farm")).isFalse();
}
+ @Test
+ public void testHStrLen() {
+jedis.hset("farm", "chicken", "little");
+
+assertThat(jedis.hstrlen("farm", "chicken")).isEqualTo("little".length());
+assertThat(jedis.hstrlen("farm", "unknown-field")).isEqualTo(0);
+assertThat(jedis.hstrlen("unknown-key", "unknown-field")).isEqualTo(0);
+ }
Review comment:
do a hstrlen on some other existing type (like a string or set) and
confirm you get the expected error
##
File path:
geode-redis/src/integrationTest/java/org/apache/geode/redis/HashesIntegrationTest.java
##
@@ -150,6 +150,15 @@ public void testHDelDeletesKeyWhenHashIsEmpty() {
assertThat(jedis.exists("farm")).isFalse();
}
+ @Test
+ public void testHStrLen() {
+jedis.hset("farm", "chicken", "little");
+
+assertThat(jedis.hstrlen("farm", "chicken")).isEqualTo("little".length());
+assertThat(jedis.hstrlen("farm", "unknown-field")).isEqualTo(0);
+assertThat(jedis.hstrlen("unknown-key", "unknown-field")).isEqualTo(0);
+ }
Review comment:
do we have a way of testing the wrong number of arguments to a command?
Will any of our redis clients even let that happen?
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] kohlmu-pivotal opened a new pull request #5242: GEODE-8190: Cleaning up tests and automating GeodeVersion build numbe…
kohlmu-pivotal opened a new pull request #5242: URL: https://github.com/apache/geode/pull/5242 …ring for tests 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] kohlmu-pivotal closed pull request #5242: GEODE-8190: Cleaning up tests and automating GeodeVersion build numbe…
kohlmu-pivotal closed pull request #5242: URL: https://github.com/apache/geode/pull/5242 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 #5184: change more string commands to have CommandFunction support
lgtm-com[bot] commented on pull request #5184: URL: https://github.com/apache/geode/pull/5184#issuecomment-642819605 This pull request **fixes 2 alerts** when merging 08f2b923f688a0dd3e50b2b29ff5400a6b0f4dbb into 4a0eeeb57ccfa59ace909b55b6f228deac7621cf - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-4e9d7fb84acaef6e6d3d54b3fc892f485d1c39df) **fixed alerts:** * 1 for Useless comparison test * 1 for Array index out of bounds 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 #5241: GEODE-8243: Use java.exe on Windows in Launcher tests
kirklund opened a new pull request #5241: URL: https://github.com/apache/geode/pull/5241 The Launcher acceptance tests fail on Windows because the tests are hardcoded to use java instead of java.exe. 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_r438938334
##
File path:
geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/MembershipIntegrationTest.java
##
@@ -172,6 +179,91 @@ 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(2_000) // expected amount of sleep time per loop in
GMSJoinLeave.join()
Review comment:
Updated in latest commits
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_r438938194
##
File path:
geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/MembershipIntegrationTest.java
##
@@ -172,6 +179,91 @@ 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(2_000) // expected amount of sleep time per loop in
GMSJoinLeave.join()
+.multipliedBy(lateJoiningMembershipLocatorPorts.length * 2); //
expected number of loops
+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) {
+e.printStackTrace();
Review comment:
Updated in latest commits
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_r438937359
##
File path:
geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/MembershipIntegrationTest.java
##
@@ -172,6 +179,91 @@ 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(2_000) // expected amount of sleep time per loop in
GMSJoinLeave.join()
Review comment:
It seems like it would be better to reference the same constants that
are used in `GMSJoinLeave` instead of hard-coding this value.
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 merged pull request #5239: GEODE-8222: Also ignore EntryDestroyedException during passive expiration
jdeppe-pivotal merged pull request #5239: URL: https://github.com/apache/geode/pull/5239 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 opened a new pull request #5240: GEODE-8242: Add HSTRLEN redis command
jdeppe-pivotal opened a new pull request #5240: URL: https://github.com/apache/geode/pull/5240 - Also define PUBSUB as an unimplemented command Authored-by: Jens Deppe 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] bschuchardt commented on a change in pull request #5236: GEODE-8241: Locator observes locator-wait-time
bschuchardt commented on a change in pull request #5236:
URL: https://github.com/apache/geode/pull/5236#discussion_r438832696
##
File path:
geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/MembershipIntegrationTest.java
##
@@ -172,6 +179,91 @@ 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(2_000) // expected amount of sleep time per loop in
GMSJoinLeave.join()
+.multipliedBy(lateJoiningMembershipLocatorPorts.length * 2); //
expected number of loops
+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) {
+e.printStackTrace();
Review comment:
There are a couple of these auto-generated try/catch statements in the
test. If they're failure conditions shouldn't the test fail? If they're not,
what good are these stack traces?
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 opened a new pull request #5239: GEODE-8222: Also ignore EntryDestroyedException during passive expiration
jdeppe-pivotal opened a new pull request #5239: URL: https://github.com/apache/geode/pull/5239 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] alb3rtobr opened a new pull request #5238: GEODE-8176: Add more checks to flaky ping test
alb3rtobr opened a new pull request #5238: URL: https://github.com/apache/geode/pull/5238 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] albertogpz commented on a change in pull request #615: GEODE-8231: remove bucket server location from metadata when server down
albertogpz commented on a change in pull request #615: URL: https://github.com/apache/geode-native/pull/615#discussion_r438587313 ## File path: cppcache/integration/test/CMakeLists.txt ## @@ -27,6 +27,7 @@ add_executable(cpp-integration-test ExampleTest.cpp ExpirationTest.cpp FunctionExecutionTest.cpp + PartitionRegionOpsWithRedundancyAndServerGoesDown.cpp Review comment: I agree with you. 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
