[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/377 Never mind. I'll create a separate PR for that. ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/377
@Randgalt Not strictly part of this PR, but I noticed that ContainerManager
doesn't log the name of the container being deleted here:
```java
try {
LOG.info("Attempting to delete candidate container: %s",
containerPath);
requestProcessor.processRequest(request);
} catch (Exception e) {
LOG.error(String.format("Could not delete container: %s" ,
containerPath), e);
}
```
The `%s` should be `{}`.
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/377 @afine This one is also an important PR to review and merge if you have a chance. I think it's already in a good shape. ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/377 @Randgalt @phunt I think this PR is in a pretty good shape, we should finalize it. Any thoughts or outstanding concerns? ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/377 FYI - I just pushed a change that adds yet-another-flag that allows 3.5.4 ZKs to read the old 3.5.3 TTL nodes. I think we must have this. The docs are updated too. ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/377 IMPORTANT NOTE: TTL Nodes created in 3.5.3 will revert to EPHEMERAL with this change. We need to discuss the impact of this and consider workarounds, etc. ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/377 Attn: @phunt - I just pushed two changes: - Better docs and new reserved bits in the `EphemeralType` enum. - Better implementation of the testable `serverId` in ZooKeeperServer. ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/377 @Randgalt i'd really like to push out a 3.5.4 - do you think it would make sense to release note this in 3.5.4 and address for 3.5.5? If you think we can finalize this PR soon I'm still open to that. ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/377 Controlling TTL Nodes via zoo.cfg turned out to be untennable. There are too many parts of the code that need to know about TTLs being enabled or not. The previous PR had several holes relating to this. This new commit changes the enabled/disabled mechanism to be a System property so that it can be accessed anywhere in the code. It's also been renamed something more general so that it can be applied to future features and not just TTLs. ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/377 @Randgalt I had to resolve some conflicts in order to compare this to current master. Posted the updated branch here: https://github.com/phunt/zookeeper/commits/ZOOKEEPER-2901 Can you review this and update this patch when you have a chance (I'm still reviewing/testing the updates at that link) ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/377 I think it needs to do the same thing whether it's in standalone mode or not. Least surprise. Also folks might want to test in standalone mode with the same basic configuration that they use with an ensemble size > 1. They'd want the same behavior. I'd say it should work the same regardless whether it's standalone or not. ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/377 > I used the sample zoo.cfg file as my config and started via bin/zkServer.sh - however I can still create a ttl based node even though "ttlNodesEnabled=false" in the zoo.cfg. I recommend you add a test for this. @phunt I imagine the server started in Standalone mode. Please verify. When the server starts in Standalone mode it ignores most of zoo.cfg. Also in Standalone mode there is no Server ID so it's a non issue. If you add `server.X` and `standaloneEnabled=false` to zoo.cfg then the ttlNodesEnabled has effect. Make sense? Does this need to be documented? ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/377 Hi @Randgalt - this looks like a reasonable change to me however it's not working. I used the sample zoo.cfg file as my config and started via bin/zkServer.sh - however I can still create a ttl based node even though "ttlNodesEnabled=false" in the zoo.cfg. I recommend you add a test for this. Can you take a look? Also I'd appreciate if you could rebase against current apache master. I did this locally manually, however I'm not sure how the submission script is going to manage it (there were conflicts because you had merged multiple times, including from third party repos (abe)). I can probably figure it out, but if you're working to address this issue perhaps you can fix this too. thx. ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/377 @phunt it's back to default false now. So, I hope this can be merged. ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/377 My "review" was a question. :-) Honestly I'm not sure what is the right thing to do here. Afraid it's painted itself into a bit of corner. ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/377 @phunt you reviewed this PR originally so I was hoping you'd merge. @rgs1 can you merge if Pat can't? ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/377 @Randgalt I've been trying to focus on getting the branches, jenkins, etc... back in shape so that I can cut some releases. Any chance your original collaborators can help? I'm kinda swamped and just getting up to speed on these will take time... ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/377 @phunt Can we get this merged please? ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/377 This feature is ready to merge (along with #378) ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/377 The integration above doesn't seem to update, but the build passes now. ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/377 FYI - I did some local ad-hoc testing with server IDs 254/255 and the config value true/false in zoo.cfg and everything works as expected. ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/377
There are 4 tests with this line of code and it's failing because the
logger is not there (NullPointerException). Any ideas?
```
Layout layout =
Logger.getRootLogger().getAppender("CONSOLE").getLayout();
```
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/377 The zoo_sample.cfg file should also be updated as part of this patch. It should include the option, and IMO it should have ttl turned on by default (similar warning wrt id as comment in the file?) ---
