You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2020/12/28 12:29:01 UTC

[GitHub] [activemq] jbonofre opened a new pull request #593: [AMQ-7502] Remove leveldb and partition

jbonofre opened a new pull request #593:
URL: https://github.com/apache/activemq/pull/593


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [activemq] jbonofre merged pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
jbonofre merged pull request #593:
URL: https://github.com/apache/activemq/pull/593


   


-- 
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:
users@infra.apache.org

[GitHub] [activemq] michaelandrepearce commented on pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-806321094


   > @michaelandrepearce did you have time to review this PR ? If you have some time to do so, it would be great. I would like to merge this PR pretty soon as it's blocking my other PRs (JDK11 build, Spring5 update, Log4j2 update, ...). Thanks !
   
   Hi @jbonofre sorry just been busy with work, haven't gone through every change, but regarding the bits around partitioning which i had raised, it looks good to me.


-- 
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:
users@infra.apache.org

[GitHub] [activemq] michaelandrepearce commented on pull request #593: [AMQ-7502] Remove leveldb and partition

Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-752034638


   Very much so. Partition is something given time something id like to port to artemis, its something when doing so would make it more an api so its not coupled as tightly with zk (e.g. zk just would be a pluggable option) but it certainly is very nice feature that helps broker setup be scaleable without manually having to shard/partition flows as a user


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [activemq] jbonofre commented on pull request #593: [AMQ-7502] Remove leveldb and partition

Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-752103502


   @michaelandrepearce ok thanks. I'm refactoring activemq-partition to avoid the dependency to leveldb and I will let there.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [activemq] lucastetreault removed a comment on pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
lucastetreault removed a comment on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-792579967


   Can one of the admins verify this patch?


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [activemq] michaelandrepearce edited a comment on pull request #593: [AMQ-7502] Remove leveldb and partition

Posted by GitBox <gi...@apache.org>.
michaelandrepearce edited a comment on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-752034638


   Very much so. Partition is something given time something id like to port to artemis, its something when doing so would make it more an api so its not coupled as tightly with zk (e.g. zk just would be a pluggable option) but it certainly is very nice feature that helps broker setup be scaleable without manually having to shard/partition flows manually as a user


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [activemq] jbonofre commented on pull request #593: [AMQ-7502] Remove leveldb and partition

Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-751960617


   @michaelandrepearce both are "related", as activemq-partition uses activemq-leveldb zookeeper client. Do you think it makes sense to keep partition ?


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [activemq] jbonofre commented on pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-794969270


   Rebasing and keeping partition.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [activemq] jbonofre commented on pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-805872217


   I'm rebasing on `master`. As said in my previous comment: can you please verify and eventually approve this PR ? It blocks me to create the dependency PRs (JDK11 build, Spring5 update, Log4j2 update, ...).


-- 
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:
users@infra.apache.org

[GitHub] [activemq] michaelandrepearce commented on a change in pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #593:
URL: https://github.com/apache/activemq/pull/593#discussion_r601002514



##########
File path: activemq-partition/src/main/java/org/apache/activemq/partition/ZKClient.java
##########
@@ -39,30 +25,32 @@
 import org.linkedin.util.clock.Timespan;
 import org.linkedin.util.concurrent.ConcurrentUtils;
 import org.linkedin.util.io.PathUtils;
-import org.linkedin.zookeeper.client.ChrootedZKClient;
-import org.linkedin.zookeeper.client.IZooKeeper;
-import org.linkedin.zookeeper.client.IZooKeeperFactory;
-import org.linkedin.zookeeper.client.LifecycleListener;
-import org.linkedin.zookeeper.client.ZooKeeperFactory;
-import org.osgi.framework.InvalidSyntaxException;

Review comment:
       ignore this, just realised its exceptions that you've removed from method throws




-- 
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:
users@infra.apache.org

[GitHub] [activemq] michaelandrepearce commented on a change in pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #593:
URL: https://github.com/apache/activemq/pull/593#discussion_r601001678



##########
File path: activemq-partition/src/main/java/org/apache/activemq/partition/ZKClient.java
##########
@@ -39,30 +25,32 @@
 import org.linkedin.util.clock.Timespan;
 import org.linkedin.util.concurrent.ConcurrentUtils;
 import org.linkedin.util.io.PathUtils;
-import org.linkedin.zookeeper.client.ChrootedZKClient;
-import org.linkedin.zookeeper.client.IZooKeeper;
-import org.linkedin.zookeeper.client.IZooKeeperFactory;
-import org.linkedin.zookeeper.client.LifecycleListener;
-import org.linkedin.zookeeper.client.ZooKeeperFactory;
-import org.osgi.framework.InvalidSyntaxException;

Review comment:
       why are we removing osgi support?




-- 
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:
users@infra.apache.org

[GitHub] [activemq] michaelandrepearce commented on pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-803049868


   Ill have a look next week on this. But sounds promising.


-- 
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:
users@infra.apache.org

[GitHub] [activemq] jbonofre commented on pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-806598505


   @michaelandrepearce @mattrpav thanks for your review guys ! I'm rebasing/fixing build and I will merge. Thanks again !


-- 
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:
users@infra.apache.org

[GitHub] [activemq] mattrpav commented on pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-805867786


   Thanks for tackling the partition feature as part of this refactor.


-- 
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:
users@infra.apache.org

[GitHub] [activemq] jbonofre commented on pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-798834470


   @michaelandrepearce I have updated the PR to keep `activemq-partition` and remove `activemq-leveldb`.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [activemq] jbonofre commented on pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-805502057


   @michaelandrepearce did you have time to review this PR ? If you have some time to do so, it would be great. I would like to merge this PR pretty soon as it's blocking my other PRs (JDK11 build, Spring5 update, Log4j2 update, ...). 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:
users@infra.apache.org

[GitHub] [activemq] michaelandrepearce edited a comment on pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
michaelandrepearce edited a comment on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-806321094


   > @michaelandrepearce did you have time to review this PR ? If you have some time to do so, it would be great. I would like to merge this PR pretty soon as it's blocking my other PRs (JDK11 build, Spring5 update, Log4j2 update, ...). Thanks !
   
   Hi @jbonofre sorry just been busy with work, haven't gone through every change in this PR, but regarding the bits around partitioning which i had raised initially, it looks good to me, you've fixed my concern.


-- 
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:
users@infra.apache.org

[GitHub] [activemq] jbonofre commented on pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-801436958


   I'm fixing the compilation issue due to rebase. I will update the PR soon.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [activemq] michaelandrepearce commented on pull request #593: [AMQ-7502] Remove leveldb and partition

Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-751698687


   I get removal of leveldb not sure why removal of broker partitioning has been lumped in at the same time 


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [activemq] michaelandrepearce edited a comment on pull request #593: [AMQ-7502] Remove leveldb and partition

Posted by GitBox <gi...@apache.org>.
michaelandrepearce edited a comment on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-752034638


   Very much so. Partition is something given time something id like to port to artemis, its something when doing so would make it more an api so its not coupled as tightly with zk (e.g. zk just would be a pluggable option) but it certainly is very nice feature that helps broker setup be scaleable without manually having to shard/partition flows manually as a user. 
   
   Probably worth if you want to remove deps on leveldb to switch the client to just zk.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [activemq] jbonofre commented on pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-806593243


   Rebasing to fix the build 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq] jbonofre commented on pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-805938447


   @mattrpav it's dependent updates (removing some useless dependencies 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:
users@infra.apache.org

[GitHub] [activemq] lucastetreault commented on pull request #593: [AMQ-7502] Remove leveldb

Posted by GitBox <gi...@apache.org>.
lucastetreault commented on pull request #593:
URL: https://github.com/apache/activemq/pull/593#issuecomment-792579967


   Can one of the admins verify this patch?


----------------------------------------------------------------
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:
users@infra.apache.org