Change number of custom vars and fix concurrent error #90
Conversation
dotsbb
commented
Mar 19, 2016
dotsbb
commented
- Supporting more than 5 Custom Variables Supporting more than 5 Custom Variables #69
- Concurrency issue when setting custom variables Concurrency issue when setting custom variables #89
| * @param maxVariables | ||
| */ | ||
| public void setMaxCustomVariables(int maxVariables){ | ||
| CustomVariables.MAX_VARIABLES = maxVariables; |
There was a problem hiding this comment.
I don't like the use of a static variables like this. I'm on mobile ATM so I can't browse code well, can't we pass this as configuration parameter before tracker init? What do you think?
There was a problem hiding this comment.
@d4rken I tried to avoid static vars, but my shot wasn't successful and elegant due to this usage of CustomVariables in TrackMe class, it knows nothing about Tracker instance.
There was a problem hiding this comment.
What about removing the limit from the custom variables object and when submitting the trackme, the trackers trims the custom variables to the allowed amount before handing it to the dispatcher?
There was a problem hiding this comment.
I think the most elegant way will be to remove limit of custom vars completely.
We just can assume that developers are responsible enough and read the doc strings.
There was a problem hiding this comment.
I agree, that sounds like a clean solution with some javadoc explanation.
Am 24.03.2016 00:39 schrieb "Alexey Subbotin" notifications@github.com:
In piwik-sdk/src/main/java/org/piwik/sdk/Piwik.java
#90 (comment):@@ -68,6 +68,16 @@ public synchronized Tracker newTracker(@nonnull String trackerUrl, int siteId) t
}/**
\* Configure Piwik to receive more than 5 custom variable\* http://piwik.org/faq/how-to/faq_17931/\* Changing this limit will affect all Tracker instances\* @param maxVariables*/- public void setMaxCustomVariables(int maxVariables){
CustomVariables.MAX_VARIABLES = maxVariables;I think the most elegant way will be to remove limit of custom vars
completely.
We just can assume that developers are responsible enough and read the doc
strings.—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/piwik/piwik-sdk-android/pull/90/files/9c1561bcf2ec4d190e9515b9fa198607815d8669#r57255556
dotsbb
force-pushed
the
change-max-custom-vars-and-fix-concurrent-error
branch
from
9c1561b
to
1c3a3da
Compare
dotsbb
force-pushed
the
change-max-custom-vars-and-fix-concurrent-error
branch
from
1c3a3da
to
01dd79e
Compare
| * @param name of a specific Custom Variable such as "User type". | ||
| * @param value of a specific Custom Variable such as "Customer". | ||
| * @return super.put result if index in right range and name/value pair aren't null | ||
| */ | ||
| public JSONArray put(int index, String name, String value) { | ||
| if (index > 0 && index <= MAX_VARIABLES && name != null & value != null) { | ||
| if (index > 0 && name != null & value != null) { |
There was a problem hiding this comment.
Wrong indexes will be ignored on the server side
|
What about wrapping an instance of HashMap instead of extending it? Would make it way easier to guard (just synchronization on 3 methods). Other calls like Map.clear() might still cause issues otherwise, I'm still on mobile, can't check the sources well. We only need a few specific methods anyways and not all HashMap has to offer. What do you think? |
|
@d4rken I've got rid of extending HashMap, as always good idea 👍 |
| */ | ||
| public class CustomVariables { | ||
| private final Object mConcurrentLock = new Object(); | ||
| private final HashMap<String, JSONArray> mVars = new HashMap<>(); |
There was a problem hiding this comment.
I think we can go with
private final Map<String, JSONArray> mVars = new ConcurrentHashMap<>();
and then skip the manual syncronization.
According to it's documentation the map iterator is safe during multithreading as long as each thread uses it's own iterator.
|
Sorry took me a bit longer. Should we do another release after this is merged into dev? On a side note, shouldn't we submit PRs via forks ;)? |
|
@d4rken all comments addressed/fixed.
Sure!
Honestly I Didn't think about it before You asked, for me it was easier to maintain only remote. |
|
👍 I think this can be merged into dev now. Then we could either do a release with that state or see if we want to include something for #92 |
