Openbravo Issue Tracking System - Retail Modules
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0033101Retail ModulesWeb POSpublic2016-06-01 12:162016-06-29 17:36
Reportermtaal 
Assigned Tomtaal 
PrioritynormalSeverityminorReproducibilityhave not tried
StatusclosedResolutionfixed 
PlatformOS5OS Version
Product Version 
Target VersionRR16Q3Fixed in Version 
Merge Request Status
Review Assigned ToAugustoMauch
OBNetwork customer
Support ticket
Regression level
Regression date
Regression introduced in release
Regression introduced by commit
Triggers an Emergency PackNo
Summary0033101: Several improvements to Multi-Server: all prefs as milli-seconds, separate thread for online transition
DescriptionThis issue combines several improvements in the multi-server behavior:
- use request retry time out also on server
- wait short time between retrying connection
- do transition to online in separate thread
- don't cache server definition, but cache the server definition id
- make sure all multi-server time prefs are in milli-seconds and have sensible defaults
Steps To ReproduceCheck multi-server code
Proposed Solution
Additional Information
TagsNo tags attached.
Relationships
Attached Files
Issue History
Date ModifiedUsernameFieldChange
2016-06-01 12:16mtaalNew Issue
2016-06-01 12:16mtaalAssigned To => mtaal
2016-06-01 12:16mtaalTriggers an Emergency Pack => No
2016-06-06 08:52hgbotCheckin
2016-06-06 08:52hgbotNote Added: 0087015
2016-06-06 08:52hgbotStatusnew => resolved
2016-06-06 08:52hgbotResolutionopen => fixed
2016-06-06 08:52hgbotFixed in SCM revision => http://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/6a6f980e374af7fabf739bda3998edcf0682135d [^]
2016-06-13 16:51mtaalReview Assigned To => AugustoMauch
2016-06-14 16:49AugustoMauchNote Added: 0087257
2016-06-14 17:21AugustoMauchNote Added: 0087259
2016-06-14 17:21AugustoMauchStatusresolved => new
2016-06-14 17:21AugustoMauchResolutionfixed => open
2016-06-16 08:37hgbotCheckin
2016-06-16 08:37hgbotNote Added: 0087324
2016-06-16 08:37hgbotStatusnew => resolved
2016-06-16 08:37hgbotResolutionopen => fixed
2016-06-16 08:37hgbotFixed in SCM revisionhttp://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/6a6f980e374af7fabf739bda3998edcf0682135d [^] => http://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/caa052599230fd23f9b798c2b17c08ae8df02249 [^]
2016-06-29 17:36AugustoMauchNote Added: 0088086
2016-06-29 17:36AugustoMauchStatusresolved => closed

Notes
(0087015)
hgbot   
2016-06-06 08:52   
Repository: erp/pmods/org.openbravo.mobile.core
Changeset: 6a6f980e374af7fabf739bda3998edcf0682135d
Author: Martin Taal <martin.taal <at> openbravo.com>
Date: Mon Jun 06 08:51:09 2016 +0200
URL: http://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/6a6f980e374af7fabf739bda3998edcf0682135d [^]

Fixes issue 33101: Several improvements to Multi-Server: all prefs as milli-seconds, separate thread for online transition
AD_REF_LIST.xml --> change description of prefs to show that milliseconds should be entered
MobileMainServerCheck.java --> use milliseconds, use request interval pref, prevent connection leak
MobileServerController.java --> make sure that also in admin mode a server can be found, cache this server id, not the server instance, move transition to online logic to separate thread
MobileServerRequestExecutor.java --> wait 50ms between retries, re-use code, solve issue that status was not recomputed
ServerStateBackground.java --> if store server only check main server, if main server only check store servers

---
M src-db/database/sourcedata/AD_REF_LIST.xml
M src/org/openbravo/mobile/core/servercontroller/MobileMainServerCheck.java
M src/org/openbravo/mobile/core/servercontroller/MobileServerController.java
M src/org/openbravo/mobile/core/servercontroller/MobileServerRequestExecutor.java
M src/org/openbravo/mobile/core/servercontroller/RetrieveMobileServerStatus.java
M src/org/openbravo/mobile/core/servercontroller/ServerStateBackground.java
---
(0087257)
AugustoMauch   
2016-06-14 16:49   
Code review:

* [1] Use constant attributed instead of hardcoded numbers
* [2] Comment is wrong, it says it defaults back to 30 seconds, but it actually is defaulting to 60. Better not even specifying the default in the comment as that might change, leaving the comment outdated
* If possible refactor the run method, it is currently 140+ lines long which makes it difficult to understand
* [3] This comment seems outdated too


[1] https://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/6a6f980e374af7fabf739bda3998edcf0682135d#l2.29 [^]
[2] https://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/6a6f980e374af7fabf739bda3998edcf0682135d#l2.43 [^]
[3] https://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/file/6a6f980e374a/src/org/openbravo/mobile/core/servercontroller/MobileMainServerCheck.java#l25 [^]
(0087259)
AugustoMauch   
2016-06-14 17:21   
More:
* [4] Wrong comment, it returns an ID not a MobileServerDefinition
* [5] getThisServerDefinition (returns a MobileServerDefinition) uses getThisServerDefinitionId (obtains a MobileServerDefinition and returns its id). Doing an OBQuery to obtain a MobileServerDefinition, then return its id and then using a OBDal.getInstance.get() to obtain the MobileServerDefinition seems overcomplex. The getThisServerDefinitionId is only used by getThisServerDefinition
* [6] Consider refactoring TransitionToOnline.run to make it more readable. Add a comment to clarify when do the handlers need to be retried.
* [7] Add Javadoc to TryConnection
* Overload the TryConnection method with a signature that does not require entering the number of previous attempts.
* Add Javadoc to handleOtherServerStatus
* [8] After add part to the where clause (store servers notify central servers of their statuses), is the previous queryPart [9] also needed?

[4] https://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/6a6f980e374af7fabf739bda3998edcf0682135d#l3.124 [^]
[5] https://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/6a6f980e374af7fabf739bda3998edcf0682135d#l3.106 [^]
[6] https://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/6a6f980e374af7fabf739bda3998edcf0682135d#l3.271 [^]
[7] https://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/file/6a6f980e374a/src/org/openbravo/mobile/core/servercontroller/MobileServerRequestExecutor.java#l113 [^]
[8] https://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/6a6f980e374af7fabf739bda3998edcf0682135d#l6.18 [^]
[9] https://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/file/6a6f980e374a/src/org/openbravo/mobile/core/servercontroller/ServerStateBackground.java#l59 [^]
(0087324)
hgbot   
2016-06-16 08:37   
Repository: erp/pmods/org.openbravo.mobile.core
Changeset: caa052599230fd23f9b798c2b17c08ae8df02249
Author: Martin Taal <martin.taal <at> openbravo.com>
Date: Thu Jun 16 08:36:30 2016 +0200
URL: http://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/caa052599230fd23f9b798c2b17c08ae8df02249 [^]

Fixes issue 33101: Several improvements to Multi-Server: all prefs as milli-seconds, separate thread for online transition
Improvements after code review

MobileMainServerCheck:
- improved class javadoc
- moved values to constants
- offline check should not check own server, added this in the whereclause in the main loop
- moved code from main loop to separate method to improve readability

MobileServerController:
- updated javadoc
- transition to online code: moved code to separate method to improve readability

MobileServerRequestExecutor:
- implemented tryConnection without retries parameter
- updated javadoc
- solved error in whereclause (forgotten quote)

MultiServerJSONProcess:
- log the exception

---
M src/org/openbravo/mobile/core/servercontroller/MobileMainServerCheck.java
M src/org/openbravo/mobile/core/servercontroller/MobileServerController.java
M src/org/openbravo/mobile/core/servercontroller/MobileServerRequestExecutor.java
M src/org/openbravo/mobile/core/servercontroller/MultiServerJSONProcess.java
---
(0088086)
AugustoMauch   
2016-06-29 17:36   
Code reviewed and verified in [1]@caa052599230

[1] https://code.openbravo.com/erp/pmods/org.openbravo.mobile.core [^]