[GitHub] [cloudstack] DaanHoogland commented on pull request #4634: Display vlan ip range for specified domainid

[GitBox <gi...@apache.org>]

DaanHoogland commented on pull request #4634:
URL: https://github.com/apache/cloudstack/pull/4634#issuecomment-919804206


   > > > > > > no comment on this PR but all this `DAO` code really shouldn't be in the `ManagementServer`
   > > > > > 
   > > > > > 
   > > > > > @DaanHoogland any idea where else I can add this ?
   > > > > 
   > > > > 
   > > > > `DomainVlanMapDaoImpl`, i'd say.
   > > > 
   > > > 
   > > > @DaanHoogland Im using the same code which is used by `account` and `pod` also which are present above and below of this code. So even they needs to be moved away?
   > > > I searched in other places in the same file and they are also using the same logic. for eg: publicipaddress, loadbalancer, resourcetag, vlan, guestos
   > > 
   > > 
   > > @ravening the rest of the code is not perfect and should be in DAOs as well. I'm not asking that of you now as I don't expect you to fix everything in one go, just to not add to the technical debt we have.
   > 
   > @DaanHoogland
   > 
   > there are more than ten Dao.search() or Dao.searchAndCount() in ManagementServerImpl.java, and much more in QueryManagerImpl.java
   > we do not need to move everything into DAO, especially for the searches in multiple (more than 2) tables.
   
   no, but those single table transactions that can we should.


-- 
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: commits-unsubscribe@cloudstack.apache.org

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