Github user romseygeek closed the pull request at:
https://github.com/apache/lucene-solr/pull/32
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the
Github user romseygeek commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-218911016
I think this is ready - will put up a patch on the JIRA issue and commit
tomorrow UK time. Which is now apparently today UK time.
/me goes to bed
---
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r62900842
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -1069,32 +1100,190 @@ public static String
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r62900820
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -1069,32 +1100,190 @@ public static String
Github user romseygeek commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r62810671
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -635,6 +669,8 @@ public Object getUpdateLock() {
Github user romseygeek commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r62810535
--- Diff:
solr/solrj/src/test/org/apache/solr/common/cloud/TestCollectionStateWatchers.java
---
@@ -0,0 +1,235 @@
+package
Github user dragonsinth commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-218321437
Almost LGTM. There's a few nits, but the only real issue is potentially
setting up a StateWatcher on legacy.
Nice work, I think we're almost done!!
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r62771180
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -1069,32 +1100,190 @@ public static String
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r62771165
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -1069,32 +1100,190 @@ public static String
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r62771056
--- Diff:
solr/solrj/src/test/org/apache/solr/common/cloud/TestCollectionStateWatchers.java
---
@@ -0,0 +1,235 @@
+package
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r62770640
--- Diff: solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java
---
@@ -154,6 +147,20 @@ public static ExecutorService
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r62770569
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -1069,32 +1100,190 @@ public static String
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r62770248
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -635,6 +669,8 @@ public Object getUpdateLock() {
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r62770279
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -635,6 +669,8 @@ public Object getUpdateLock() {
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r62770082
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -485,6 +506,20 @@ private void refreshLegacyClusterState(Watcher
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r62769742
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -485,6 +506,20 @@ private void refreshLegacyClusterState(Watcher
Github user romseygeek commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-218251017
That's... a good point, actually.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project
Github user dragonsinth commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-218226081
Hmm, isn't an executor a fancier way of doing a Queue + Thread(s)? :)
---
If your project is set up for it, you can reply to this email and have your
reply
Github user romseygeek commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-218122269
New plan! We now have a separate notification thread, and change
notifications are placed into a LinkedBlockingQueue that the thread waits on.
A caveat:
Github user dragonsinth commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-217984743
I did like the idea of a dedicated executor for collection events, just to
ensure clean separation. But I'll take a look in its current form.
---
If your
Github user romseygeek commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-217978029
On further reflection, I've pulled the separate executor back out again. I
think the SolrZkClient's separate executor will work well enough, and for the
most
Github user markrmiller commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-217291207
> Although, thinking more about that, we already have a separate executor
for watchers, don't we?
Yes, every watch firing event should run from a
Github user romseygeek commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-216941787
Last push exposes CollectionStateWatcher directly again, and moves
notification calls into an Executor.
---
If your project is set up for it, you can reply to
Github user dragonsinth commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-216931783
Correct, the queuing implementation where the waiting thread loops only
helps waitForState(). Maybe we should just go with that for now and consider
making CSW
Github user romseygeek commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-216922168
> One thing I noticed in writing this is that it's uncertain whether you'll
miss any states or not
Unless I'm misunderstanding you, this is just how ZK
Github user dragonsinth commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-216351404
BTW, here's an implementation of waitForState() that does the work on the
calling thread. This passes your tests:
```
public void
Github user romseygeek commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-216347644
Feedback is good :-)
I'll pull CSW back out and make it public again. I think keeping it
separate from the Predicate is still a useful distinction
Github user dragonsinth commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-216333963
@romseygeek nice job on the changes so far, and sorry to have so much
feedback and so many asks. This is a pretty complicated change so I feel like
it merits
Github user romseygeek commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-216315281
OK, latest push moves all notifications out of synchronized blocks.
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user romseygeek commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61777874
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -1066,32 +1079,201 @@ public static String
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61644721
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -491,19 +493,28 @@ private void
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61643877
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -256,9 +257,10 @@ public void updateClusterState() throws
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61643749
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/CollectionStatePredicate.java
---
@@ -30,8 +30,9 @@
/**
* Check the
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61643539
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -1066,32 +1079,201 @@ public static String
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61622572
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/CollectionStateWatcher.java ---
@@ -0,0 +1,42 @@
+package
Github user romseygeek commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61569303
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -1066,32 +1079,201 @@ public static String
Github user romseygeek commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61568081
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/CollectionStateWatcher.java ---
@@ -0,0 +1,42 @@
+package org.apache.solr.common.cloud;
Github user romseygeek commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-215646194
Thanks for the comments! I'll try and incorporate your suggestions and see
how far we get.
---
If your project is set up for it, you can reply to this email and
Github user romseygeek commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61541928
--- Diff:
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java ---
@@ -348,7 +358,13 @@ public JettySolrRunner
Github user romseygeek commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61541687
--- Diff:
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java ---
@@ -572,6 +574,40 @@ public void downloadConfig(String
Github user dragonsinth commented on the pull request:
https://github.com/apache/lucene-solr/pull/32#issuecomment-215576790
Looking good, a little more high-level feedback. @shalinmangar I think you
should take a look also.
Have you run the tests extensively? The first time
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61510208
--- Diff:
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java ---
@@ -348,7 +358,13 @@ public JettySolrRunner
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61510100
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/CollectionStateWatcher.java ---
@@ -0,0 +1,42 @@
+package
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61509937
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -1066,32 +1079,201 @@ public static String
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61509699
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -1066,32 +1079,201 @@ public static String
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61508998
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -1066,32 +1079,201 @@ public static String
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61507961
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -131,6 +132,19 @@
private final Runnable
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61507382
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java ---
@@ -484,6 +498,12 @@ private void refreshLegacyClusterState(Watcher
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61504824
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java ---
@@ -210,6 +213,38 @@ public Replica getReplica(String coreNodeName)
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61504670
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/CollectionStateWatcher.java ---
@@ -0,0 +1,42 @@
+package
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61504017
--- Diff:
solr/solrj/src/java/org/apache/solr/common/cloud/CollectionStatePredicate.java
---
@@ -0,0 +1,41 @@
+package
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/32#discussion_r61503724
--- Diff:
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java ---
@@ -572,6 +574,40 @@ public void downloadConfig(String
GitHub user romseygeek opened a pull request:
https://github.com/apache/lucene-solr/pull/32
SOLR-8323
Adds a CollectionStateWatcher API to listen for changes to collection state
(SOLR-8323)
You can merge this pull request into a Git repository by running:
$ git pull
53 matches
Mail list logo
