You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/09/21 14:10:58 UTC
[GitHub] [geode] ringles opened a new pull request #6887: Geode 9567 geode for redis rename
ringles opened a new pull request #6887:
URL: https://github.com/apache/geode/pull/6887
Rename "APIs Compatible with Redis" to "Geode for Redis". Affects module names, directory names, config and docs.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6887: GEODE-9567: Geode for Redis rename
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6887:
URL: https://github.com/apache/geode/pull/6887#discussion_r714222380
##########
File path: geode-for-redis/README.md
##########
@@ -97,41 +97,41 @@ not connected>
## <a name="security"></a>Security
-Security is implemented slightly differently to OSS Redis. Redis stores password information in plain text in the redis.conf file.
+Security is implemented slightly differently from OSS Redis. Redis stores password information in plain text in the redis.conf file.
-When using Apache Geode, to enable security, a Security Manager needs to be configured on the server(s). This Security Manager will authenticate `AUTH <password>` commands and `AUTH <username> <password>` commands. Users can set a custom `default` username using the `geode-compatible-with-redis-username` parameter. This username will be used when `AUTH <password>` commands are sent without a `<username>`.
+When using Apache Geode, you must configure a Security Manager on the server(s) to enable security. The Security Manager authenticates `AUTH <password>` commands and `AUTH <username> <password>` commands. Users can set a custom `default` username using the `geode-geode-for-redis-username` parameter. The default username is used when `AUTH <password>` commands are sent without a `<username>`.
Review comment:
Typo here, "geode-geode-for-redis-username" should be "geode-for-redis-username"
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #6887: GEODE-9567: Geode for Redis rename
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #6887:
URL: https://github.com/apache/geode/pull/6887#discussion_r721763743
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java
##########
@@ -294,6 +294,16 @@ private void setEventSeqNum() {
}
}
+ @Override
Review comment:
It does look like merge/rebase issue.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on a change in pull request #6887: GEODE-9567: Geode for Redis rename
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on a change in pull request #6887:
URL: https://github.com/apache/geode/pull/6887#discussion_r721645747
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java
##########
@@ -294,6 +294,16 @@ private void setEventSeqNum() {
}
}
+ @Override
Review comment:
> there are changes for the stats.
@jchen21 the stat changes you are describing seem to have come from unrelated PR https://github.com/apache/geode/pull/6199
maybe there is just some issue with merge/rebase?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ringles commented on a change in pull request #6887: GEODE-9567: Geode for Redis rename
Posted by GitBox <gi...@apache.org>.
ringles commented on a change in pull request #6887:
URL: https://github.com/apache/geode/pull/6887#discussion_r713237673
##########
File path: geode-core/src/test/resources/org/apache/geode/management/internal/cli/commands/golden-help-offline.properties
##########
@@ -2572,7 +2572,7 @@ SYNTAX\n\
\ \ \ \ [--lock-memory(=value)?] [--log-level=value] [--max-connections=value] [--max-heap=value]\n\
\ \ \ \ [--max-message-count=value] [--max-threads=value] [--mcast-address=value] [--mcast-port=value]\n\
\ \ \ \ [--memcached-port=value] [--memcached-protocol=value] [--memcached-bind-address=value]\n\
-\ \ \ \ [--compatible-with-redis-port=value] [--compatible-with-redis-bind-address=value] [--compatible-with-redis-password=value]\n\
+\ \ \ \ [--geode-for-redis-port=value] [--geode-for-redis-bind-address=value] [--geode-for-redis-password=value]\n\
Review comment:
OK, I'll see if it can be safely tanked.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6887: GEODE-9567: Geode for Redis rename
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6887:
URL: https://github.com/apache/geode/pull/6887#discussion_r713420598
##########
File path: geode-docs/developing/geode_for_redis.html.md.erb
##########
@@ -49,20 +49,20 @@ Replace `<serverName>` with the name of your server.
Replace `<locatorPort>` with your locator port.
-Replace `<compatibleWithRedisPort>` with the port that the <%=vars.product_name%> server
+Replace `<geodeForRedisPort>` with the port that the <%=vars.product_name%> server
listens on for Redis commands. The typical port used with a Geode for Redis cluster is 6379.
-Replace `<compatibleWithRedisBindAddress>` with the address of the server host.
+Replace `<geodeForRedisBindAddress>` with the address of the server host.
Replace `<compatibleWithWithRedisPassword>` with the password clients use to authenticate.
Review comment:
This also needs to be updated. Easy to miss since it's a typo.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #6887: GEODE-9567: Geode for Redis rename
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #6887:
URL: https://github.com/apache/geode/pull/6887#discussion_r721592601
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java
##########
@@ -294,6 +294,16 @@ private void setEventSeqNum() {
}
}
+ @Override
Review comment:
The code here is not simply a rename of something, as the title of the pull request suggests. In the pull request, there are changes for the stats. I would suggest put the stat code change in a separate pull request, instead of including them in the 600+ files diff, which are mostly about renaming.
I am not sure on what scenario this `startGet()` and `endGet()` on `BucketRegion` would be invoked. Why `startGet()` returns `0`, while `endGet()` does nothing. This doesn't seem right to me. And this overrides the method in `LocalRegion`. Maybe I am missing something. I can't find any description in the JIRA or pull request for stat related changes. Would you please explain a bit?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on a change in pull request #6887: GEODE-9567: Geode for Redis rename
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on a change in pull request #6887:
URL: https://github.com/apache/geode/pull/6887#discussion_r713233155
##########
File path: CODEOWNERS
##########
@@ -284,7 +284,7 @@ geode-assembly/**/resources/** @boglesby @kir
#-----------------------------------------------------------------
# Redis API
#-----------------------------------------------------------------
-geode-apis-compatible-with-redis/** @jdeppe-pivotal @nonbinaryprogrammer @ringles @upthewaterspout @DonalEvans @dschneider-pivotal @kirklund
+geode-for-redis/** @jdeppe-pivotal @nonbinaryprogrammer @ringles @upthewaterspout @DonalEvans @dschneider-pivotal @kirklund
Review comment:
please maintain indentation when changing CODEOWNERS
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ringles closed pull request #6887: GEODE-9567: Geode for Redis rename
Posted by GitBox <gi...@apache.org>.
ringles closed pull request #6887:
URL: https://github.com/apache/geode/pull/6887
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6887: GEODE-9567: Geode for Redis rename
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6887:
URL: https://github.com/apache/geode/pull/6887#discussion_r713174026
##########
File path: geode-apis-compatible-with-redis/src/acceptanceTest/resources/0001-configure-redis-tests.patch
##########
@@ -1,2786 +0,0 @@
-diff --git a/tests/support/server.tcl b/tests/support/server.tcl
Review comment:
I think this file was accidentally removed.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ringles commented on a change in pull request #6887: GEODE-9567: Geode for Redis rename
Posted by GitBox <gi...@apache.org>.
ringles commented on a change in pull request #6887:
URL: https://github.com/apache/geode/pull/6887#discussion_r713176696
##########
File path: geode-apis-compatible-with-redis/src/acceptanceTest/resources/0001-configure-redis-tests.patch
##########
@@ -1,2786 +0,0 @@
-diff --git a/tests/support/server.tcl b/tests/support/server.tcl
Review comment:
Oy. That's weird. I'll get it back 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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6887: GEODE-9567: Geode for Redis rename
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6887:
URL: https://github.com/apache/geode/pull/6887#discussion_r713167954
##########
File path: geode-core/src/test/resources/org/apache/geode/management/internal/cli/commands/golden-help-offline.properties
##########
@@ -2572,7 +2572,7 @@ SYNTAX\n\
\ \ \ \ [--lock-memory(=value)?] [--log-level=value] [--max-connections=value] [--max-heap=value]\n\
\ \ \ \ [--max-message-count=value] [--max-threads=value] [--mcast-address=value] [--mcast-port=value]\n\
\ \ \ \ [--memcached-port=value] [--memcached-protocol=value] [--memcached-bind-address=value]\n\
-\ \ \ \ [--compatible-with-redis-port=value] [--compatible-with-redis-bind-address=value] [--compatible-with-redis-password=value]\n\
+\ \ \ \ [--geode-for-redis-port=value] [--geode-for-redis-bind-address=value] [--geode-for-redis-password=value]\n\
Review comment:
I'm pretty sure this file can just be deleted - I don't see any references to it anywhere. Even before your changes it looks like it is referencing `geode-for-redis-password` which is no longer a valid param (I missed this reference when I removed that parameter).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #6887: GEODE-9567: Geode for Redis rename
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on pull request #6887:
URL: https://github.com/apache/geode/pull/6887#issuecomment-924137023
Hmmm - there are exactly `666` files changed....
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ringles commented on a change in pull request #6887: GEODE-9567: Geode for Redis rename
Posted by GitBox <gi...@apache.org>.
ringles commented on a change in pull request #6887:
URL: https://github.com/apache/geode/pull/6887#discussion_r713954429
##########
File path: geode-docs/developing/geode_for_redis.html.md.erb
##########
@@ -49,20 +49,20 @@ Replace `<serverName>` with the name of your server.
Replace `<locatorPort>` with your locator port.
-Replace `<compatibleWithRedisPort>` with the port that the <%=vars.product_name%> server
+Replace `<geodeForRedisPort>` with the port that the <%=vars.product_name%> server
listens on for Redis commands. The typical port used with a Geode for Redis cluster is 6379.
-Replace `<compatibleWithRedisBindAddress>` with the address of the server host.
+Replace `<geodeForRedisBindAddress>` with the address of the server host.
Replace `<compatibleWithWithRedisPassword>` with the password clients use to authenticate.
Review comment:
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ringles commented on a change in pull request #6887: GEODE-9567: Geode for Redis rename
Posted by GitBox <gi...@apache.org>.
ringles commented on a change in pull request #6887:
URL: https://github.com/apache/geode/pull/6887#discussion_r713233818
##########
File path: CODEOWNERS
##########
@@ -284,7 +284,7 @@ geode-assembly/**/resources/** @boglesby @kir
#-----------------------------------------------------------------
# Redis API
#-----------------------------------------------------------------
-geode-apis-compatible-with-redis/** @jdeppe-pivotal @nonbinaryprogrammer @ringles @upthewaterspout @DonalEvans @dschneider-pivotal @kirklund
+geode-for-redis/** @jdeppe-pivotal @nonbinaryprogrammer @ringles @upthewaterspout @DonalEvans @dschneider-pivotal @kirklund
Review comment:
Will do!
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org